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

fix(plugin-vite): Don't copy node_modules on build #3579

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joeyballentine
Copy link

@joeyballentine joeyballentine commented Apr 26, 2024

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

This resolves the ongoing issues with the Vite plugin (as well as fixes #3570). The plugin right now copies all dependencies, all the time, regardless of config. This is absolutely insane as it completely bypasses Vite's normal bundling and tree-shaking process in favor of copying all node_modules and requiring them (via registering them as external), instead of inlining the tree-shaken code. This means you can get something like my case, where ~200mb of uncompressed, uncompiled node modules were being copied, slowing down my build time, .zip extract time, and dramatically increasing my bundle size.

This PR fixes that issue by simply removing the offending code. It is not needed. Anyone who needs to copy a specific node module for whatever reason can use something like this Vite plugin to simply copy their node module manually. However, for 99.9% of use cases, this will be unnecessary.

This also brings the Vite plugin more in line to how the webpack one works (which also does not copy any node modules by default)

There is one issue with this though: any import or require that is not top-level does not get inlined, and must either be moved to the top-level or have its node_modules copied. This appears to just be standard Vite behavior and is different from webpack. It is worth noting that because this, this PR actually does break electron-squirrel-startup imports, assuming a user is following the recommended guide for its usage, since that does a require inside the condition of an if statement. This can simply just be moved to the top to avoid this problem (I've tested it, it works fine), but it is important to note that it would technically be a breaking change until a user fixes that themselves. However, I do believe the benefits outweigh the negatives here.

Users will also have to remove the offending dependency externalizations from their configs as well, so that could be considered another breaking change.

@joeyballentine joeyballentine requested a review from a team as a code owner April 26, 2024 22:16
@erickzhao erickzhao requested review from erickzhao, caoxiemeihao and a team April 26, 2024 23:15
@caoxiemeihao
Copy link
Member

If you use C/C++ addons, such as sqlite3 in your Vite template project. How would you build this without copying node_modules?

@joeyballentine
Copy link
Author

joeyballentine commented Apr 28, 2024

by copying those node modules specifically using a copy plugin and marking them as external. Some modules needing to have this done is not an excuse to apply this to everything.

Back when I was using webpack, we had to use a copy plugin to bundle some wasm with our code. I'm very glad webpack didn't force all my node modules to be bundled raw with my app just because of one package being silly.

@joeyballentine
Copy link
Author

And for the record, there appears to be a vite plugin that deals with exactly what you're talking about: https://github.com/vite-plugin/vite-plugin-native

@caoxiemeihao
Copy link
Member

caoxiemeihao commented Apr 28, 2024

And for the record, there appears to be a vite plugin that deals with exactly what you're talking about: https://github.com/vite-plugin/vite-plugin-native

vite-plugin-native currently handles C/C++ addons very roughly.

@caoxiemeihao
Copy link
Member

I think this PR is a breaking update and we'll discuss how to more appropriately support user control over tow node_modules are copied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to Vite plugin to filter copied dependencies
2 participants