feat(plugin-vite): better logic #3583

caoxiemeihao · 2024-04-30T06:06:11Z

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.

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)
Remove plugin-vite copy dependencies to node_modules of built App. (fix(plugin-vite): Don't copy node_modules on build #3579)

caoxiemeihao · 2024-04-30T06:07:29Z

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

joeyballentine · 2024-05-02T15:37:22Z

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 · 2024-05-02T19:20:42Z

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 · 2024-05-03T02:57:41Z

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 · 2024-05-03T04:56:11Z

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 · 2024-05-17T07:53:33Z

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 · 2024-05-17T12:59:21Z

it would be a PITA to manually whitelist

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

BurningEnlightenment · 2024-05-17T16:29:21Z

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.

packages/plugin/vite/package.json

packages/plugin/vite/src/ViteConfig.ts

packages/plugin/vite/src/config/vite.main.config.ts

packages/template/vite/tmpl/forge.config.js

…gic BUG

jgresham · 2024-07-31T00:48:29Z

@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

packages/plugin/vite/src/config/vite.base.config.ts

BlackHole1

LGTM

BlackHole1 · 2024-09-13T10:59:12Z

PTAL @electron/forgers

BurningEnlightenment · 2024-09-18T08:24:36Z

packages/plugin/vite/src/ViteConfig.ts

+ const userConfig = (await loadConfigFromFile(configEnv, buildConfig.config))?.config ?? {};
+ switch (target) {
+ case 'main':
+ return mergeConfig(getMainViteConfig(configEnv as ConfigEnv<'build'>), userConfig);
+ case 'preload':
+ return mergeConfig(getPreloadViteConfig(configEnv as ConfigEnv<'build'>), userConfig);
+ case 'renderer':
+ return mergeConfig(getRendererViteConfig(configEnv as ConfigEnv<'renderer'>), userConfig);
+ default:
+ throw new Error(`Unknown target: ${target}, expected 'main', 'preload' or 'renderer'`);
+ }


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.

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

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 · 2024-09-18T08:34:23Z

packages/plugin/vite/src/VitePlugin.ts

- await fs.writeJson(path.resolve(buildPath, 'package.json'), pj, {
- spaces: 2,
- });
-
- // Copy the dependencies in package.json
- for (const dep of flatDependencies) {
- await fs.copy(dep.src, path.resolve(buildPath, dep.dest));
- }


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?

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.

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.

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.

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.

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 · 2024-09-18T23:47:53Z

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

caoxiemeihao requested a review from a team as a code owner April 30, 2024 06:06

caoxiemeihao force-pushed the feat/vite-builtin-config branch from 7fc7a19 to 23aca91 Compare April 30, 2024 08:28

erickzhao marked this pull request as draft April 30, 2024 17:08

caoxiemeihao force-pushed the feat/vite-builtin-config branch from a517c24 to 34dc5a7 Compare May 4, 2024 00:16

erickzhao mentioned this pull request May 14, 2024

fix(plugin-vite): add vite as a peer dep #3591

Closed

5 tasks

This comment was marked as off-topic.

Sign in to view

caoxiemeihao changed the title ~~WIP(refactor(plugin-vite): better logic)~~ refactor(plugin-vite): better logic Jun 23, 2024

caoxiemeihao force-pushed the feat/vite-builtin-config branch 2 times, most recently from 366fdb8 to 6774799 Compare June 28, 2024 07:33

caoxiemeihao marked this pull request as ready for review June 29, 2024 05:29

caoxiemeihao requested review from erickzhao, a team and BlackHole1 June 30, 2024 02:16

caoxiemeihao linked an issue Jul 1, 2024 that may be closed by this pull request

Undocumented breaking change for Vite plugin in v7.3.0 #3506

Closed

3 tasks

BlackHole1 requested changes Jul 5, 2024

View reviewed changes

caoxiemeihao force-pushed the feat/vite-builtin-config branch 3 times, most recently from 2c00b69 to 660b192 Compare July 8, 2024 06:05

caoxiemeihao requested a review from BlackHole1 July 8, 2024 06:47

caoxiemeihao added 3 commits July 8, 2024 17:30

refactor(plugin-vite): better \node_modules\ collect strategy

5485969

refactor(plugin-vite): move vite config from template to plugin

34c6446

feat(plugin-vite): load built in vite config

c701336

caoxiemeihao added 9 commits July 8, 2024 17:30

fix(plugin-vite): return undefined avoid packagerConfig.ignore lo…

182fa3f

…gic BUG

chore(plugin-vite): remove process reference

bb9ee55

feat(plugin-vite): remove copy dependencies to built App

1bae6ab

chore(plugin-vite): update config

75815ac

chore(plugin-vite): test add vite.base.config.ts

da1ba8d

fix(template-vite): remove vite.base.config.ts

29d4e0b

chore(plugin-vite): cleanup code style

5880756

fix(plugin-vite): necessary forge config options

a0148b5

chore(plugin-vite): update test

9c6ca78

BlackHole1 force-pushed the feat/vite-builtin-config branch from 660b192 to 9c6ca78 Compare July 8, 2024 09:30

BlackHole1 requested changes Jul 31, 2024

View reviewed changes

packages/plugin/vite/src/config/vite.base.config.ts Show resolved Hide resolved

Merge branch 'main' into feat/vite-builtin-config

1ae795f

caoxiemeihao requested a review from BlackHole1 September 13, 2024 09:58

BlackHole1 approved these changes Sep 13, 2024

View reviewed changes

BurningEnlightenment reviewed Sep 18, 2024

View reviewed changes

VerteDinde approved these changes Sep 18, 2024

View reviewed changes

VerteDinde changed the title ~~refactor(plugin-vite): better logic~~ feat(plugin-vite): better logic Sep 18, 2024

Merge branch 'main' into feat/vite-builtin-config

ea21a2a

georgexu99 added this pull request to the merge queue Sep 19, 2024

erickzhao approved these changes Sep 20, 2024

View reviewed changes

Merged via the queue into electron:main with commit 83fa9cf Sep 20, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(plugin-vite): better logic #3583

feat(plugin-vite): better logic #3583

caoxiemeihao commented Apr 30, 2024 •

edited

Loading

caoxiemeihao commented Apr 30, 2024

joeyballentine commented May 2, 2024 •

edited

Loading

erickzhao commented May 2, 2024

caoxiemeihao commented May 3, 2024 •

edited

Loading

joeyballentine commented May 3, 2024

BurningEnlightenment commented May 17, 2024

joeyballentine commented May 17, 2024

BurningEnlightenment commented May 17, 2024 •

edited

Loading

This comment was marked as off-topic.

jgresham commented Jul 31, 2024

BlackHole1 left a comment

BlackHole1 commented Sep 13, 2024

BurningEnlightenment Sep 18, 2024 •

edited

Loading

caoxiemeihao Sep 18, 2024

BurningEnlightenment Sep 18, 2024

BurningEnlightenment Sep 18, 2024

caoxiemeihao Sep 18, 2024 •

edited

Loading

BurningEnlightenment Sep 18, 2024

caoxiemeihao Sep 18, 2024

caoxiemeihao Sep 18, 2024

caoxiemeihao Sep 18, 2024

VerteDinde commented Sep 18, 2024

feat(plugin-vite): better logic #3583

feat(plugin-vite): better logic #3583

Conversation

caoxiemeihao commented Apr 30, 2024 • edited Loading

caoxiemeihao commented Apr 30, 2024

joeyballentine commented May 2, 2024 • edited Loading

erickzhao commented May 2, 2024

caoxiemeihao commented May 3, 2024 • edited Loading

joeyballentine commented May 3, 2024

BurningEnlightenment commented May 17, 2024

joeyballentine commented May 17, 2024

BurningEnlightenment commented May 17, 2024 • edited Loading

This comment was marked as off-topic.

jgresham commented Jul 31, 2024

BlackHole1 left a comment

Choose a reason for hiding this comment

BlackHole1 commented Sep 13, 2024

BurningEnlightenment Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

caoxiemeihao Sep 18, 2024

Choose a reason for hiding this comment

BurningEnlightenment Sep 18, 2024

Choose a reason for hiding this comment

BurningEnlightenment Sep 18, 2024

Choose a reason for hiding this comment

caoxiemeihao Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

BurningEnlightenment Sep 18, 2024

Choose a reason for hiding this comment

caoxiemeihao Sep 18, 2024

Choose a reason for hiding this comment

caoxiemeihao Sep 18, 2024

Choose a reason for hiding this comment

caoxiemeihao Sep 18, 2024

Choose a reason for hiding this comment

VerteDinde commented Sep 18, 2024

caoxiemeihao commented Apr 30, 2024 •

edited

Loading

joeyballentine commented May 2, 2024 •

edited

Loading

caoxiemeihao commented May 3, 2024 •

edited

Loading

BurningEnlightenment commented May 17, 2024 •

edited

Loading

BurningEnlightenment Sep 18, 2024 •

edited

Loading

caoxiemeihao Sep 18, 2024 •

edited

Loading