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

feat: experimental Node.js imports compatibility for client-side #25028

Merged
merged 24 commits into from Jan 18, 2024

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Jan 3, 2024

πŸ”— Linked issue

resolves #12786, #25016

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR allows using libraries for code that depends on Node.js API by polyfilling them using unjs/unenv which is already used for server bundles (via nitro and presets).

Notes:

  • All node: and node imports are supported
  • No global injections are supported. process, Buffer, etc needs to be manually imported

Injecting globals:

To avoid accidentally increasing bundle size if only a code-syntax depends on Buffer check (that adds ~42Kb, and Vue deps already do this!), auto injections are disabled. But they can be easily imported from node:

import { Buffer } from 'node:buffer'
import process from 'node:process'

Inside a Nuxt plugin, they can be also polyfilled (by User's risk!)

// plugins/node.client.ts
import { Buffer } from 'node:buffer'
import process from 'node:process'

globalThis.Buffer = Buffer
globalThis.process = process

In next iterations we might find smarter ways to safely inject them.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

@pi0 pi0 requested review from antfu and danielroe January 3, 2024 12:07
Copy link

stackblitz bot commented Jan 3, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the 3.x label Jan 3, 2024
test/bundle.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

I love this. I am wondering if we should default it to false, however - so it can be an escape hatch for users who need it.

I am thinking it would still be better for users to be aware of the fact that they are using node dependencies in the browser, so they can potentially bail out or switch to a different library.

What do you think?

@pi0
Copy link
Member Author

pi0 commented Jan 3, 2024

I am also happy to keep it disabled by default feel free to commit before merge. But it kinda takes away the point of build-in integration because users can already easily do same unenv client support from nuxt.config and alias

Currently we do same mocks on server-side for non Nodejs targets it makes the behavior more consistant for library support when universally imported in SSR.

Meanwhile i agree about keeping users aware of that, i guess as a followup we can show a warn about dependencies that use Node.js API and guiding users about prefering an alternative lib (if they are lucky enough to have alt!) with dep graph this should be straight forward. In many cases such aliases will be tree-shaken also BTW but they are only in dependency graph of rollup.

@pi0 pi0 marked this pull request as ready for review January 3, 2024 17:30
@pi0
Copy link
Member Author

pi0 commented Jan 3, 2024

(i am also not happy about top level unenv key as it doesn't indicate client config. any idea welcome!)

@danielroe danielroe added this to the 3.10 milestone Jan 3, 2024
@danielroe
Copy link
Member

danielroe commented Jan 3, 2024

Okay, that's a persuasive reason to enable by default. But I'd suggest there might be merit in logging or warning users when polyfills are injected...

On the schema, what if we use a single unenv key with client/server subkeys that configures unenv? (We pass the server config across to ntiro..., and the client config to vite/webpack...)

We could also consider making the key more generic, as the average user who wants to configure this might be unaware of unenv or what it is or does...

Finally, if there's more testing to be done, we could release this under experimental as an opt-in before later enabling as default...

@pi0 pi0 changed the title feat: mock Node.js api for client-side using unenv feat: experimental Node.js imports compatibility for client-side Jan 3, 2024
@pi0
Copy link
Member Author

pi0 commented Jan 3, 2024

Mind you, in the current state what we do is only enable access to node: (and without proto) imports which previously would only make an error for the vite client-side bundle and there is no injection magic. It is all up to the user.

While I think it should be something really safe to be enabled by default, I have moved it behind experimental.clientNodeCompat flag. Also, the flag is now only a boolean as the only effect (addition of aliases) is configurable by users using the same alias config to override some or more.

Copy link

nuxt-studio bot commented Jan 18, 2024

βœ… Live Preview ready!

Name Edit Preview Latest Commit
Nuxt Docs Edit on Studio β†—οΈŽ View Live Preview c85c279

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

beautiful. πŸ‘Œ

I would actually be fine enabling by default (with a warning if needed) given there's no bundle cost - but let's see usage and we can always enable in future.

@danielroe danielroe merged commit dab2188 into main Jan 18, 2024
36 checks passed
@danielroe danielroe deleted the feat/client-node-polyfills branch January 18, 2024 16:09
@github-actions github-actions bot mentioned this pull request Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support unenv for client to polyfill Node.js imports
3 participants