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.

Sign up for GitHub

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

Jump to bottom

fix(dev): exclude node:-prefixed built-ins from the browser build #5222

Closed
wants to merge 2 commits into from
Closed

fix(dev): exclude node:-prefixed built-ins from the browser build #5222

wants to merge 2 commits into from

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented Jan 23, 2023 •
edited
Loading

Closes: #4544

  • Docs
  • Tests

TODO

@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 5b1437e

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

@pcattori pcattori changed the title Pedro/node prefixed built ins fix(dev): exclude node:-prefixed built-ins from the browser build Jan 23, 2023
sergiodxa
sergiodxa approved these changes Jan 23, 2023
View reviewed changes
Copy link
Member

@sergiodxa sergiodxa left a comment

Choose a reason for hiding this comment

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

Will this also allows import Buffer from "buffer" to import buffer from the node_modules? Or is this just to consider node:* as server-only imports?

@pcattori pcattori force-pushed the pedro/node-prefixed-built-ins branch from fd7453e to 50b3070 Compare January 23, 2023 16:42
mcansh
mcansh reviewed Jan 23, 2023
View reviewed changes
packages/remix-dev/compiler/compileBrowser.ts Outdated
Comment on lines 38 to 45
let nodeBuiltins = Array.from(
new Set([
...builtinModules,
// account for `node:`-prefixed built-ins like `"node:fs"`
...builtinModules.map((x) => `node:${x}`),
])
);
Copy link
Collaborator

@mcansh mcansh Jan 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i'm sort of surprised that builtinModules doesn't already account for the node:* prefix tbh 🤷

Copy link
Member

Choose a reason for hiding this comment

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

It does, but not sure since what version that's included tbh

https://github.com/nodejs/node/blob/762a3a8ad925d56a12b43e0e7f7c811e93097784/lib/internal/modules/cjs/loader.js#L311

Copy link
Member

Choose a reason for hiding this comment

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

This was also introduced in v16.17 (see nodejs/node#43396), so we should account for values that can or can not have node:-prefix

A possible solution would be to map over all values, remove the node:-prefix, put that array in a Set so we have unique values, loop over the Set to add the node:-prefix again

@pcattori pcattori marked this pull request as draft January 23, 2023 17:29
@pcattori
Copy link
Contributor Author

pcattori commented Jan 23, 2023 •
edited
Loading

This is blocked on polyfills not handling for node: prefixes. Currently, without polyfills for node:-prefix the app will have console errors.

remorses/esbuild-plugins#20

@pcattori pcattori force-pushed the pedro/node-prefixed-built-ins branch 2 times, most recently from 8b49d66 to 5c83a37 Compare January 23, 2023 17:34
This was referenced Jan 23, 2023
Merged
Merged
@MichaelDeBoey
Copy link
Member

@pcattori #5274 unblocks this PR again 🎉

@pcattori
Copy link
Contributor Author

@MichaelDeBoey thanks for looking into it!

I thought about this some more and we shouldn't be using polyfills for Node in the browser in the first place. I have a branch locally that treeshakes out all Node built-ins (but keeps them if they are actually 3rd party libs like buffer that are installed in your project), but haven't had time to put that up yet due to HMR focus recently.

@MichaelDeBoey MichaelDeBoey force-pushed the pedro/node-prefixed-built-ins branch from 5c83a37 to 5b1437e Compare March 9, 2023 01:00
@MichaelDeBoey MichaelDeBoey mentioned this pull request Mar 12, 2023
Merged
2 tasks
@pcattori
Copy link
Contributor Author

Superceded by #5773

@pcattori pcattori closed this Mar 22, 2023
@pcattori pcattori deleted the pedro/node-prefixed-built-ins branch March 22, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcansh mcansh mcansh left review comments

@MichaelDeBoey MichaelDeBoey MichaelDeBoey approved these changes

@sergiodxa sergiodxa sergiodxa approved these changes

Assignees
No one assigned
Labels
CLA Signed package:dev
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pcattori @MichaelDeBoey @sergiodxa @mcansh