-
Notifications
You must be signed in to change notification settings - Fork 31
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
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 |
@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 |
@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 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. |
…th `rollup-plugin-polyfill-node`
b626b97
to
66964e3
Compare
@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. |
Try using this plugin instead https://github.com/cyco130/esbuild-plugin-polyfill-node This PR doesn't work |
@remorses All tests are passing, so it should work now I guess? Can you point me to what is still failing? |
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) |
Re-submission of #19
This still has the same problem as @connorjclark pointed out in #19 (comment) (see also #28 for the bug issue)
@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