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

Ensure Symbol.dispose and Symbol.asyncDispose are available #15404

Merged
merged 5 commits into from
Dec 16, 2024

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Dec 16, 2024

We recently introduced some better instrumentation (#15303) which uses the new using keyword. I made sure that this was compiled correctly for environments where using is not available yet.

The issue is that this also relies on Symbol.dispose being available. In my testing on our minimal required Node.js version (18) it did work fine. However, turns out that I was using 18.20.x locally where Symbol.dispose is available, but on older version of Node.js 18 (e.g.: 18.17.x) it is not available. This now results in some completely broken builds, e.g.: when running on Cloudflare Pages. See: #15399

I could reproduce this error in CI, by temporarily downgrading the used Node.js version to 18.17.0. See:

image

Implementing the proper polyfill, as recommended by the TypeScript docs ( see: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-2.html#:~:text=Symbol.dispose,-??=%20Symbol(%22Symbol.dispose ), the error goes away. (If you look at CI after the polyfill, it still fails but for different reasons unrelated to this change)

Fixes: #15399


Test plan

  1. I reproduced it in CI, and I kept the commits so that you can take a look where it fails with the Object not disposable.
  2. Using the provided reproduction from v4: Vite build fails on Cloudflare #15399:

Before

It works on Node.js v18.20.x, but switching to Node.js v18.17.x you can see it fail:

image

After

Using pnpm's overrides, we can apply the fix from this PR and test it in the reproduction. You'll notice that it now works in both Node.js v18.20.x and v18.17.x

image

`process.stdout.write` returns a boolean by default, but for the type of
the flush method we don't care about the return type.
@RobinMalfait RobinMalfait force-pushed the fix/issue-15399 branch 4 times, most recently from bcb8aca to b382399 Compare December 16, 2024 11:59
This should bring us past the `Object not disposable` error due to the
`Symbol.dispose` and `Symbol.asyncDispose` not being available.
@RobinMalfait RobinMalfait marked this pull request as ready for review December 16, 2024 12:31
@RobinMalfait RobinMalfait requested a review from a team as a code owner December 16, 2024 12:31
@RobinMalfait RobinMalfait changed the title Ensure Symbol.dipose and Symbol.asyncDispose are available Ensure Symbol.dispose and Symbol.asyncDispose are available Dec 16, 2024
@@ -763,3 +765,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Move the CLI into a separate `@tailwindcss/cli` package ([#13095](https://github.com/tailwindlabs/tailwindcss/pull/13095))

## [4.0.0-alpha.1] - 2024-03-06

Copy link
Contributor

Choose a reason for hiding this comment

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

prettier I guess?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, was removed in some previous PR for some reason.

I think we should actually just add an empty placeholder or remove this empty header 😅

Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

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

I did not realize that TypeScript doesn't polyfill these itself — welp. Glad we caught it now.

@RobinMalfait RobinMalfait merged commit 352d1b9 into next Dec 16, 2024
5 checks passed
@RobinMalfait RobinMalfait deleted the fix/issue-15399 branch December 16, 2024 13:17
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.

v4: Vite build fails on Cloudflare
2 participants