WIP(refactor(plugin-vite): better logic)

[caoxiemeihao]

• 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:

In flighting!

The main purpose of this PR is to solve the two issues currently encountered without breaking changes.

1. It will be compatible with versions <7.2.0 and >=7.3.0 of Forge. (Undocumented breaking change for Vite plugin in v7.3.0 #3506)
2. Users can configure packagerConfig.ignore to prevent plugin-vite from copying node_modules. (fix(plugin-vite): Don't copy node_modules on build #3579)
3. Remove plugin-vite copy dependencies to node_modules of built App. (fix(plugin-vite): Don't copy node_modules on build #3579)~~

[caoxiemeihao]

This is not a mandatory PR to be merged, it's only for discussion.

[joeyballentine]

Users can configure packagerConfig.ignore to prevent plugin-vite from copying node_modules

I still fail to understand why the default behavior you want is to copy everything and ignore specific things rather than the other way around. Please, try to help me understand why this is preferable when only a few modules actually need to be copied (or perhaps none at all, like in my project). Vite's default behavior is to bundle everything together and tree shake. What is the point of using vite if you are going to ignore half of what it does?

Edit: if the answer is to avoid breaking changes, I would argue that we should make this breaking change, as it was done incorrectly to begin with. It would be better to right this wrong sooner rather than later, and save countless people in the future from bundling their app incorrectly.

[erickzhao]

Regarding this implementation, I think I have two comments:

• I agree with @joeyballentine that node_modules should by default and find a more elegant solution forward.
• I think that we might want to add vite back as a direct dependency to the plugin (or at least in peerDependencies) to avoid confusing users since vite is notably not going to be present for people who used the template from v7.2.0 and are upgrading to a future version.

[caoxiemeihao]

I still fail to understand why the default behavior you want is to copy everything and ignore specific things rather than the other way around. Please, try to help me understand why this is preferable when only a few modules actually need to be copied (or perhaps none at all, like in my project). Vite's default behavior is to bundle everything together and tree shake. What is the point of using vite if you are going to ignore half of what it does?

The first misconception is that not all node_modules are collected according to the dependencies rule.
The second misconception is that Tree Shake only works on esm format bundled(deps) code, and has no effect on cjs formatting(deps). All codes final minify(trim trees) depends on esbuild or terser.

If we exclude all dependencies to copy to node_modules, who can help users build C/C++ native modules?
By the way, I also hope not to collect dependencies. But this will have a certain impact on previous users. Is this acceptable?

[joeyballentine]

The first misconception is that not all node_modules are collected according to the dependencies rule.

Sorry, I keep saying "everything" when I mean "all dependencies". Dev dependencies will not be copied, of course.

The second misconception is that Tree Shake only works on esm format bundled code

Are you sure about this? My project is not configured to be an ES module and yet tree shaking works perfectly fine.

If we exclude all dependencies to copy to node_modules, who can help users build C/C++ native modules?

What exactly is the difference between a user knowing that they should ignore their dependencies and a user knowing they should specifically copy their native modules? Either way, a user has to know that they need extra configuration. Since native code is rarer, i do not think a default rule favoring it is preferrable, personally.

Side-note: I will admit that I was previously unaware that are legitimate projects that put all their dependencies in devDependencies. There's apparently a line of thinking that all dependencies for a non-library are dev dependencies since the user does not need to install them and they are only needed for the build. I had not personally encountered this and based on my (admittedly limited) nodejs experience this is different from the norm. However, that seems to be more common in vite land? So while I might disagree with it, it is probably what vite users expect.

I'm mostly just concerned about this due to the mismatch between the webpack plugin and the vite plugin. Maybe someone starting with the vite plugin would know to put all their dependencies into devDependencies because that's what vite people do, whereas a webpack user migrating to vite wouldn't. In that case, I would suggest a migration guide to help mitigate what I ran into, (which was wasting hours trying to figure out why it was bundling code i didn't want). At the very least, proper practice for ensuring your dependencies get processed properly should be documented.

Anyway, I don't wanna be one of those guys who just relentlessly argues on the internet about something. So, I will not make any more comments on this topic as a whole except for making replies.

I think whatever you decide to do is fine, and if it does not work for me and my project I can always just continue to patch the plugin.

[BurningEnlightenment]

What exactly is the difference between a user knowing that they should ignore their dependencies and a user knowing they should specifically copy their native modules? Either way, a user has to know that they need extra configuration. Since native code is rarer, i do not think a default rule favoring it is preferrable, personally.

The main problem are things like prebuild-install which is used by quite a few popular libraries e.g. sqlite3. It's main purpose is to download built native modules from the internet and locate them at runtime. Due to the last part you have to include the whole prebuild-install package in your distribution even though most of it is effectively dead code. And this goddamn package comes with a rather big dependency tree, i.e. it would be a PITA to manually whitelist. And yes, it's an incredibly stupid library design… 🤬

[joeyballentine]

it would be a PITA to manually whitelist

And it's less of a PITA to manually blacklist?

[BurningEnlightenment]

And it's less of a PITA to manually blacklist?

I usually rely on @electron/packager's prune option to do the heavy lifting, i.e. I just have to maintain a list of problematic dependencies (=> package.json:dependencies) and the tooling tracks the transitive dependency graph required to run the app.

EDIT: It would perhaps be useful if @electron/forge and @electron/packager provided a transformPackageJson(value: object): object option for mangling the package.json. This would also easily allow for removing stuff like scripts and devDependencies during packaging.