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

fix: circumvent issues with ESM process polyfills #543

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

WasabiThumb
Copy link

@WasabiThumb WasabiThumb commented Oct 14, 2024

Would resolve #539. This is a hacky solution that may preferrably be made obsolete by resolution of davidmyersdev/vite-plugin-node-polyfills#106.

The root issue is that some node:process polyfills export an ES module that is not unwrapped at runtime. This change would introduce a patch that conditionally unwraps the module in the event that checks pass indicating that it is wrapped. Those checks are:

  • require("node:process")["__esModule"] === true
    • Intent is to quickly eliminate false positives
  • "nextTick" in require("node:process")["default"] || "nextTick" in require("node:process")["process"]
    • Indicates that the default or process property of the import is indeed intended to polyfill node:process. The choice of nextTick here is arbitrary, however the method would be required for proper function of this package.

This also inherits the curiosity introduced by #497 where node:process is imported via process/.

@WasabiThumb WasabiThumb changed the title Circumvent issues with ESM process polyfills fix: circumvent issues with ESM process polyfills Oct 14, 2024
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.

Thanks for opening a PR! Can you please add a unit test?

@WasabiThumb
Copy link
Author

WasabiThumb commented Oct 23, 2024

Thanks for opening a PR! Can you please add a unit test?

Sorry for the delay! I've added the task test:vite. It was very painful to replicate the correct setup, but it works like a charm in the end. It's not as graceful as the other browser tests, mostly because I couldn't get playwright working on Arch, but I believe this is a decent starting point. Specifically, the use of ViteDevServer.openBrowser() instead of playwright may not play well with CI. However, IPC is performed using an endpoint on the dev server rather than console stdout; therefore migration should be easy for those that can get playwright to run should that be desired. On the other hand, the need to test multiple browsers is currently met by the rollup suite.

Example output:
image

@WasabiThumb
Copy link
Author

WasabiThumb commented Oct 23, 2024

This is strange- despite using the reprex as a reference, the test does not break when commenting out the patching functionality of the shim introduced by this PR. It's possible that the issue only crops up when the Vite source is TypeScript, since that's the only obvious difference. I may have to consider updating the test to use TypeScript- or perhaps throw this all out and consider it a TypeScript issue.

EDIT: Or maybe Vite is caching the version of readable-stream with the patch uncommented? I'm not sure how to test this rigorously- but it does make sense for it to make that sort of optimization, and we're not telling it to invalidate this cache (if it exists). 🤔🤔🤔 Perhaps those following at home can try this with an untainted environment.

EDIT 2: Made the apparent version change randomly from run to run in an attempt to bust any caches that may exist; same behavior.

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.

Incompatibility with Rollup/Vite process polyfill
2 participants