feat(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).

Up until v7.2.0, @electron-forge/plugin-vite was designed to minimize user boilerplate, with all required Vite configurations already integrated into the plugin itself. While this approach provided a good starting experience for users, it also had a few downsides:

• Baking the configurations into the plugin made it difficult to implement additional features without breaking Forge's existing design principles.
• Vite's rapid release cadence made it harder to keep the Forge plugin up to date with the latest version.

In #3468 (released in Forge v7.3.0), I shifted a lot of the configuration logic from @electron-forge/plugin-vite to @electron-forge/template-vite, which enabled useful features such as hot restart, hot reload, and improved ESM support (which is important for frameworks such as Svelte) to be implemented with slight tweaks to the template code.

However, the changes introduced in v7.3.0 broke builds for existing users of @electron-forge/plugin-vite (see #3506). Based on community feedback brought by users of both v7.2.0 and v7.3.0, I am trying to release a new version that will have the advantages of both iterations of the plugin and not introduce breaking changes between updates.

What changes did this PR make? First of all, let’s make it clear that the underlying Vite configs of v7.2.0 and v7.3.0 are almost identical, except that they are located in different places. So, if we keep the v7.3.0 config as-is and move it to @electron-forge/plugin-vite, we can solve the problem we are facing now. This is exactly what this PR does.

┏————————————————————————————————————————————————————┓
│      base.config.mjs, main, preload, renderer      │
┗————————————————————————————————————————————————————┛
                          ↓
                      🚚 🚚 🚚 🚚
                          ↓
┏————————————————————————————————————————————————————┓
│ @electron-forge/plugin-vite/config/base.config.mjs │
┗————————————————————————————————————————————————————┛

In addition, since v7.2.0 does not have Hot Restart and Hot Reload features,
this PR provides a { target?: 'main' | The 'preload' } option is used to enable them, but it is not required.

{
  name: '@electron-forge/plugin-vite',
  config: {
    build: [
      {
        entry: 'src/main.js',
        config: 'vite.main.config.js',
+       target: 'main',
      },
      {
        entry: 'src/preload.js',
        config: 'vite.preload.config.js',
+       target: 'preload',
      },
    ],
  },
}

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. 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.

[jgresham]

@caoxiemeihao is there a way we could test the changes our for you? just excited for the updates and curious when it might be out? thanks!

and thank you for your work on this @caoxiemeihao and advocacy @joeyballentine

[BlackHole1]

LGTM

[BlackHole1]

PTAL @electron/forgers

[BurningEnlightenment]

Can we get a bypass? I started with 7.4.0 and have modified the vite.base.config.mjs. Mainly to provide a root if the config gets loaded by vitest and for vite-tsconfig-paths. Without a bypass this would be a breaking change.

EDIT: Has it been considered to provide the vite extensions and default configurations as a library? That would allow me to pluck things like root from the default config and I can call mergeConfig afterwards myself.

[caoxiemeihao]

vite.base.config.mjs in your project will have higher priority than @electron-forge/plugin-vite, and your configuration will still be valid.

[BurningEnlightenment]

That is only true for things which I explicitly configure, e.g. if I use a different way to implement hot-restart, I'd have to override the plugin applied by default to do nothing… which also looks rather awkward. Therefore it would be nice, if I could just import the pieces I want from this plugin and apply them by myself.

[BurningEnlightenment]

Am I reading this correctly and this removes the functionality to copy dependencies listed in package.json? Is this supposed to land in a forge v8 package or will this breaking change land in a minor version?

[caoxiemeihao]

This should be released as a minor version, because Vite does not build some Node.js modules reliably.
However, most people still want Vite to handle the Node.js modules in dependencies, which can effectively reduce the size of the App build.
In any case, the current change is the choice of most people. If other problems are encountered during the upgrade process, we may need some community-provided Vite plugins to handle them instead of forge.

[BurningEnlightenment]

In any case, the current change is the choice of most people.

And how has this been measured?

If other problems are encountered during the upgrade process, we may need some community-provided Vite plugins to handle them instead of forge.

Do you mean a vite plugin to include the native binaries in the bundle?

In any case why is it necessary to break existing configuration now? AFAICT it would be possible to add an option to toggle this behavior.

[caoxiemeihao]

In general, people who have used @electron-forge/plugin-webpack plugin hope that vite-plugin can have the same design as webpack-plugin, so that they can easily migrate from Webpack to Vite. But in fact, Vite and Webpack are not compatible in many places, and we can only work hard to make it more like Webpack. At least the current PR behavior is consistent with Webpack.
We may need to remove some hack behaviors from Forge, and they will be done by Vite plugin. For example, the copy behavior of dependencies looks very stupid in the eyes of Webpack users, but Vite often cannot solve such problems, especially C/C++ native packages.
Of course, I personally think that the current version of Vite plugin is the result of weighing many aspects, but it is not easy to be accepted by Webpack plugin users.

[caoxiemeihao]

For users, the upgrade will bring some additional problems, which is not only a problem faced by Forge users, but more accurately, it should be a problem that Vite needs to face. I will try my best to provide some additional plugins to help users solve these problems, and also to provide Vite ecosystem with more Webpack-like capabilities.

[caoxiemeihao]

In any case why is it necessary to break existing configuration now? AFAICT it would be possible to add an option to toggle this behavior.

Adding an option to let the user decide whether to copy dependencies would be necessary, as this PR is clearly a departure from previous behavior.
Please let me know if anyone agrees to add this option?

[VerteDinde]

Updated the PR from a refactor to a feat, so that we have a minor bump. I think this is a large enough change that we should alert users with a minor