refactor(remix-dev): replace esbuild-plugin-polyfill-node with esbuild-plugins-node-modules-polyfill

[LukeAskew]

esbuild-plugin-polyfill-node was introduced as part of #5274. Unfortunately, that plugin introduces some issues:

• It includes globals unnecessarily (Esbuild inject adds all node globals for non-node environment #6380)
• Per this comment, it increases the build size due to the inclusion of globals.
• There is a potential for conflict with other globals defined by downstream module code due to the strategy it uses for including globals. See attached screenshot.

This change replaces esbuild-plugin-polyfill-node with esbuild-plugins-node-modules-polyfill, which also uses jspm (via modern-node-polyfills).

---

Closes: #6380

Testing Strategy: Replaced plugin in my remix app and did not encounter errors.

[changeset-bot]

⚠️ No Changeset found

Latest commit: adc3621

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[remix-cla-bot]

Hi @LukeAskew,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[ivogt]

I've noticed that the esbuild-plugin-polyfill-node also polyfills node:async_hooks etc (makes them unavailable with a stub unimplemented). Basically making compatibility_flags = [ "nodejs_compat" ] unusable.

Is this PR handling these correctly and we can use nodejs_compat?

[LukeAskew]

@ivogt If I understand correctly this should only affect browser code so you should be able to use node:async_hooks in Cloudflare's compat mode since it's Node.js. I'm not sure how you'd implement async_hooks for browser code. (jspm/jspm-core#28)

[ivogt]

@LukeAskew Sorry, let me clarify. So, for Cloudflare Workers the build is handled as 'neutral'. Since workers support nodejs_compat and there are couple of modules with node: prefix (e.g. node:utils and node:async_hooks which these polyfills override. In https://github.com/remix-run/remix/pull/6414/files#diff-833c098a2a1bb29fb7094fb9a15af9f299cadbd1fb35bcc0cd7f078807fa8358 we remove the imports of node:*module* which prevents us from using them.

I suggest we exclude the workers from the polyfill e.g. ctx.config.serverPlatform !== "node" && ctx.config.serverPlatform !== "neutral" .

Don't think we need to polyfill for workers runtime, do we?

Thank you

[ivogt]

Also mentioned here #6348

[bryce-pearce]

this would also likely close #6259

#5858
#6280
#6284
#6290

as well as a few more I believe

[frandiox]

Thanks for the PR. This should also close #6348 thanks to the last commit.

[DaniBitZesty]

Hello, I'm working on a project that's quite dependent on this fix. Will this be merged soon? 🙏

[BrycePearce]

Hello, I'm working on a project that's quite dependent on this fix. Will this be merged soon? 🙏

likewise

[MichaelDeBoey]

Superseded by #6547

[jacob-ebey]

I have broken this PR up into two parts:

1. Replace the polyfill package: refactor(remix-dev): replace esbuild-plugin-polyfill-node with esbuild-plugins-node-modules-polyfill #6562
2. Add config option for if polyfills are used or not: feat(remix-dev): introduce serverNodeBuiltinsPolyfill config option #6547

We can't accept this as is as it changes existing behavior for users. Thank you for the contribution and pointing us at a better alternative :)

[jacob-ebey]

Part 1 merged: #6562

[brandonpittman]

Testing Strategy: Replaced plugin in my remix app and did not encounter errors.

I did.