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(node-modules-polyfill): replace rollup-plugin-node-polyfills with rollup-plugin-polyfill-node #29

Conversation

MichaelDeBoey
Copy link
Contributor

Re-submission of #19

This still has the same problem as @connorjclark pointed out in #19 (comment) (see also #28 for the bug issue)

Cannot read directory "\x00polyfill-node._stream_duplex": invalid argument

@remorses @connorjclark If you have any idea what the problem is & can point me into the right direction, I'm happy to update this PR

Maybe the problem is even deeper & we should look at @FredKSchott's rollup-plugin-polyfill-node to solve it? 🤔


Tagging @connorjclark @giuseppeg @ice1000 @imranbarbhuiya @i7eo @matikucharski @p-m-p as you all were interested in my first PR

Closes #18 for real now

@remorses
Copy link
Owner

remorses commented Jan 28, 2023

The problem is that to use those polyfill you have to reimplement this resolver: https://github.com/FredKSchott/rollup-plugin-polyfill-node/blob/main/src/index.ts#L40

That's because the polyfills are importing code from those paths

I am not really a rollup expert so i don't know how to translate it to esbuild 1 to 1

@MichaelDeBoey
Copy link
Contributor Author

to use those polyfill you have to reimplement this resolver: https://github.com/FredKSchott/rollup-plugin-polyfill-node/blob/main/src/index.ts#L40

I am not really a rollup expert so i don't know how to translate it to esbuild 1 to 1

@FredKSchott could you maybe point us into the right direction?

@remorses Can you think of any other solution to fix it? Maybe we shouldn't use rollup-plugin-polyfill-node but have the polyfills in another way available?

@p-m-p
Copy link

p-m-p commented Jan 31, 2023

@MichaelDeBoey I presume you tagged me as I referenced this from my issue on the Remix repo.

I did look at your PR here when I raised that issue and think if you add another onLoad in the setup to namespace the modules prefixed with \0polyfill-node. and resolve them like is done here https://github.com/FredKSchott/rollup-plugin-polyfill-node/blob/main/src/index.ts#L75 it could probably work with some tweaking. Not all of the modules are in the modules map, some exist only in the exports of the polyfills file.

Probably not a helpful observation at this point but I do think it would be more beneficial to extract the polyfills properly into an esbuild plugin that is maintained in the Remix org.

@MichaelDeBoey MichaelDeBoey force-pushed the replace-node-polyfills-with-maintained-version branch from b626b97 to 66964e3 Compare March 29, 2023 16:49
@MichaelDeBoey
Copy link
Contributor Author

MichaelDeBoey commented Mar 29, 2023

@remorses I included @p-m-p's suggestions & test from #32, so I think this one's good to be merged in?

I'll try to talk to the Remix team about forking this package into the monorepo, but I don't they want to do so tbh.
The added test is proving that internal \0polyfill-node.* imports (which come from rollup-plugin-polyfill-node) are resolved properly, so I think we can merge this PR with confidence it won't break any other things.

@remorses
Copy link
Owner

remorses commented Apr 5, 2023

Try using this plugin instead https://github.com/cyco130/esbuild-plugin-polyfill-node

This PR doesn't work

@MichaelDeBoey
Copy link
Contributor Author

@remorses All tests are passing, so it should work now I guess?

Can you point me to what is still failing?
I'm happy to update this PR

@remorses
Copy link
Owner

remorses commented Apr 6, 2023

Just use the plugin i linked above, the polyfill library used in this PR isn't much better than the one already used, the plugin i linked instead uses an actively maintained polyfill (jspm)

sxzz added a commit to vuejs/core that referenced this pull request Apr 29, 2023
yyx990803 pushed a commit to vuejs/core that referenced this pull request May 1, 2023
@MichaelDeBoey MichaelDeBoey deleted the replace-node-polyfills-with-maintained-version branch June 8, 2023 18:47
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.

[node-modules-polyfill] Replace unmaintained rollup-plugin-node-polyfills dependency
3 participants