Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Bun and Deno to typescript support check #408

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Codex-
Copy link

@Codex- Codex- commented Aug 24, 2024

This change allows the Bun and Deno runtimes to use this loader with Fastify base without erroring as seen in #317.

Tests

Node

No regressions, everything passing.

Bun

  • bun run typescript
  • bun run typescript:jest
  • bun run typescript:vitest
  • bun run unit
    • unit.js updated to consume an arg for the runtime
  • The bun equivalent of the loader tests that execute test/typescript/basic.ts cannot be run as a particular function Tap uses has not yet been implemented in Bun
    • NotImplementedError: node:v8 Serializer is not yet implemented in Bun.

Deno

  • deno task typescript
  • Deno implements ServerResponse as a class, while node a function, which causes this error in Fastify.
    • Class constructor ServerResponse cannot be invoked without 'new'
    • This causes the following to fail
      • deno task typescript:jest
      • deno task typescript:vitest
      • deno task unit

While it has already been communicated that Fastify is not actively supporting either Bun or Deno runtimes, despite the results, it would be great to see this check updated to allow their usage at a "ymmv" degree.

Fixes #317

Checklist

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we do not support Bun or Deno on Fastify.
Frankly, we have not time, and I would not accept a PR without tests either.

https://github.com/fastify/fastify/blob/main/docs/Reference/LTS.md#long-term-support

@Codex- Codex- force-pushed the typescript_runtime_bun_deno branch from 2c0b43d to 5a6d882 Compare August 24, 2024 15:43
@Codex-
Copy link
Author

Codex- commented Aug 24, 2024

Currently we do not support Bun or Deno on Fastify.

While I understand that is the official stance, as noted in your link, with all of the other supported tooling already included would it really do any harm to simply allow this check to pass for these runtimes?

and I would not accept a PR without tests either.

You beat me to it, I amended my commit with the bun tests I intended to push originally :)

Fastify is simply incompatible with Deno currently, I'm happy to include another test run file to run against deno but it will fail and not add value

@Codex- Codex- requested a review from mcollina August 24, 2024 15:53
scripts/unit.js Outdated

const child = exec('npm run unit:with-modules')
const child = exec(argv[2] + ' run unit:with-modules')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add npm as a default?

You'd need to add bun to CI as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure can

I was going to ask actually, so the CI currently uses that shared workflow from the workflows repo, so to not muddy concerns in that workflow, would you be happy with an additional job added to the ci.yml that performs the tests for bun?

jobs:
  test:
    uses: fastify/workflows/.github/workflows/[email protected]
    with:
      license-check: true
      lint: true
  bun:
    uses: ...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with adding a job to the existing workflow for now, the erroneously published version aplha takes precedence over alpha in string ordering so until it reaches beta the workflow needs to specifically install the version required for tests to satisfy the semver usage (since semver doesn't recognise aplha as a valid version)

@Codex- Codex- requested a review from mcollina August 25, 2024 03:36
@Codex- Codex- force-pushed the typescript_runtime_bun_deno branch from 348d44e to 150fafa Compare August 25, 2024 04:16
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@Codex- Codex- requested a review from Eomm August 25, 2024 11:34
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

# The erraneous typod version `5.0.0-aplha` takes priority over .3 as 'p' > 'l'
run: |
bun i --ignore-scripts
bun i -D --ignore-scripts [email protected]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ideal

The typod version seems to be unpublished now — can't find it at https://www.npmjs.com/package/fastify?activeTab=versions

Can you try removing the line?

Suggested change
bun i -D --ignore-scripts [email protected]

Copy link
Author

@Codex- Codex- Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't unpublish a package from npm:

image

https://www.npmjs.com/package/fastify/v/5.0.0-aplha.1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but I don't understand why this is needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the typo in the version name bun takes p as being of a higher version than l when parsing aplha

aplha.1 does not pass the tests

Can be removed once beta has begun

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm this is the case

@MrEAlderson
Copy link

Please merge, this is the only problem I have faced regarding bun support in fastify.
For anyone else stumbling on this: Adding FASTIFY_AUTOLOAD_TYPESCRIPT=true to your .env is a temporary fix

@gurgunday
Copy link
Member

Resolved the conflict but it's failing now, I'll check why when I get home

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fastify Autoload does not work with Bun.js or deno
5 participants