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

dev/build silently externalizes some dependencies, leading to runtime errors #7105

Closed
7 tasks done
NfNitLoop opened this issue Feb 27, 2022 · 8 comments
Closed
7 tasks done

Comments

@NfNitLoop
Copy link

NfNitLoop commented Feb 27, 2022

Describe the bug

Vite has some resolver plugins that cause it to "externalize" some dependencies by name. Instead of giving an error at bundle/build time, the dependency is just dropped from the dependency graph.

// node built-ins.
// externalize if building for SSR, otherwise redirect to empty module
if (isBuiltin(id)) {

This leaves you to get runtime errors when things run in the browser.

If Vite had errors (or even warnings) about this, the user could look for other libraries or polyfills to resolve the issue.

Reproduction

NfNitLoop/vite-issue@5150380

System Info

n/a.

Used Package Manager

npm

Logs

When running npm run build && npm run preview:

Uncaught TypeError: Cannot read properties of undefined (reading 'from')
at vendor.93cea733.js:1:1014
at vendor.93cea733.js:1:1703

When running npm run dev:

[vite] connecting...
index.js:12 Uncaught TypeError: Cannot read properties of undefined (reading 'from')
at node_modules/safe-buffer/index.js (index.js:12:12)
at __require (bs58check.js?v=f7e2e0ea:10:50)
at node_modules/hash-base/index.js (index.js:2:14)
at __require (bs58check.js?v=f7e2e0ea:10:50)
at node_modules/md5.js/index.js (index.js:3:16)
at __require (bs58check.js?v=f7e2e0ea:10:50)
at node_modules/create-hash/browser.js (browser.js:3:11)
at __require (bs58check.js?v=f7e2e0ea:10:50)
at node_modules/bs58check/index.js (index.js:3:18)
at __require (bs58check.js?v=f7e2e0ea:10:50)
client.ts:58 [vite] connected.

Validations

@NfNitLoop
Copy link
Author

For comparison, if I forget to enable polyfills in SnowPack, it gives me nice errors like this:

[snowpack] node_modules/readable-stream/lib/_stream_readable.js
Module "events" (Node.js built-in) is not available in the browser. Run Snowpack with --polyfill-node to fix.
[snowpack] node_modules/readable-stream/lib/_stream_readable.js
Module "buffer" (Node.js built-in) is not available in the browser. Run Snowpack with --polyfill-node to fix.

@jianqi-jin
Copy link
Contributor

@NfNitLoop I created a PR for this. :), See if it can solve your problem.

@NfNitLoop
Copy link
Author

That’s much better!

My personal preference would be an error, that the user could then bypass with some configuration. Like: externalLibs: [ 'buffer' ]

that way you can’t accidentally miss the message.

However, a warning is a definite improvement over being silent! 😊

@NfNitLoop
Copy link
Author

And of course, the nicest would be if Vite had a polyfillNode feature like Snowpack. It would make migrations from snowpack to Vite easier. But I think that’s a different ticket. 😊

@jianqi-jin
Copy link
Contributor

This sounds like a big change. It's literally a feature rather than a bug?

@Niputi
Copy link
Contributor

Niputi commented Feb 28, 2022

polyfills in vite core will never be a builtin functionality.
better error messages are fine, would especially be good in build mode.
dependencies should be fixed to not include node builtins as they don't exist in the browser and preferably export correct esm version with .mjsas extension

@NfNitLoop
Copy link
Author

NfNitLoop commented Mar 2, 2022

This sounds like a big change. It's literally a feature rather than a bug?

I agree! That's why I said:

But I think that’s a different ticket. 😊

I'm OK with just adding warning messages.


dependencies should be fixed to not include node builtins

In an ideal world, sure. But that's not the current state of npm packages in my experience. There are many old packages that work just fine in the browser if they're given a Buffer implementation, for example. I ran into a few grandchild (grandparent?) dependencies like this in attempting to port a project from SnowPack to Vite.

Anyway, as I said above, adding polyfills is a separate issue. For the scope of this issue, I'm happy with just adding some warnings/errors.


I should elaborate:

There are many old packages that work just fine in the browser if they're given a Buffer implementation, for example.

... and they don't have obvious replacements. (Or, the thing I'm depending on, which depends on them, doesn't have an obvious/easy replacement.)

At which point my choice becomes

  • take on maintenance/reimplementation of some of my dependencies because they don't work with Vite
  • Stick with my previous bundler that works fine with those dependencies
  • Become so fed up with JS/npm that I go try Flutter. 😆

@bluwy
Copy link
Member

bluwy commented Dec 30, 2022

The latest Vite version should have better warnings when using builtin modules in the browser. I believe the improvements came in Vite 3

@bluwy bluwy closed this as not planned Won't fix, can't repro, duplicate, stale Dec 30, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants