electron / forge

:electron: A complete tool for building and publishing Electron applications
https://electronforge.io
MIT License
6.32k stars 489 forks source link

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

Open joeyballentine opened 1 month ago

joeyballentine commented 1 month ago

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.

caoxiemeihao commented 1 month ago

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

joeyballentine commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.