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

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

Open
3 tasks done
erickzhao opened this issue Feb 23, 2024 · 31 comments
Open
3 tasks done

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

erickzhao opened this issue Feb 23, 2024 · 31 comments

Comments

@erickzhao
Copy link
Member

Pre-flight checklist

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project uses.
  • I have searched the issue tracker for a bug that matches the one I want to file, without success.

Electron Forge version

7.3.0

Electron version

N/A

Operating system

All

Last known working Electron Forge version

7.2.0

Expected behavior

Upgrading your Vite plugin app from Electron Forge v7.2.0 to v7.3.0 is seamless.

Actual behavior

Changes in the build system during the upgrade to Vite 5 have produced an API-incompatible change, requiring users to upgrade.

Steps to reproduce

N/A

Additional information

To fix this issue, see the @electron-forge/template-vite template config files: https://github.com/electron/forge/tree/main/packages/template/vite/tmpl

@phlegmaticprogrammer

This comment has been minimized.

@OmegaRogue

This comment has been minimized.

@will-stone
Copy link

Do you mind pointing to the change that fixed the issue? There seems to be a lot going on there. Here’s my files: https://github.com/will-stone/browserosaurus

@sparecycles
Copy link

sparecycles commented Mar 25, 2024

Chiming in here... I've been using configs based on modified 7.2.0 templates (vite + electron + svelte! 😉 ).

Upgrading to 7.3.0 causes my existing configs to ... just not produce any output! (.vite/build/... empty: no main.js found).

I think I might be able to get things to work if I rebuild the changes in my templates on top of the new ones, but that seems involved for a minor release.

Note: I don't think it's the vite@5 update that's causing my issue, as my package.json already specified "vite": "^5.2.6"

@Ben-Avrahami

This comment was marked as duplicate.

@sparecycles
Copy link

sparecycles commented Mar 28, 2024

The only insight I didn't mention is that the main PR description ( #3468 ) admits

... this PR change is very significant. Moving most of the logic form [sic] plugin/vite to template/vite ...

So, I think the expectation here is we recreate our build configs on the new base templates?

@boldt
Copy link

boldt commented Apr 16, 2024

Can you provide a migration guide, to fix this issue?

@Ben-Avrahami
Copy link

Can you provide a migration guide, to fix this issue?

+1 !

markspolakovs added a commit to ystv/badger that referenced this issue Apr 17, 2024
@caoxiemeihao
Copy link
Member

caoxiemeihao commented Apr 20, 2024

Migrate to 7.3.0+

Updated on 2024-04-25 (electron-forge-plugin-vite@0.6.1)


vite.main.config.ts

  import { defineConfig } from 'vite';
+ import { forgeViteConfig } from 'electron-forge-plugin-vite/migration';

  // https://vitejs.dev/config
- export default defineConfig({
+ export default defineConfig(forgeViteConfig.main({
    resolve: {
      // Some libs that can run in both Web and Node.js, such as `axios`, we need to tell Vite to build them in Node.js.
      browserField: false,
      conditions: ['node'],
      mainFields: ['module', 'jsnext:main', 'jsnext'],
    },
- });
+ }));

vite.renderer.config.ts

  import { defineConfig } from 'vite';
+ import { forgeViteConfig } from 'electron-forge-plugin-vite/migration';

  // https://vitejs.dev/config
- export default defineConfig({});
+ export default defineConfig(forgeViteConfig.renderer({}));

vite.preload.config.ts

  import { defineConfig } from 'vite';
+ import { forgeViteConfig } from 'electron-forge-plugin-vite/migration';

  // https://vitejs.dev/config
- export default defineConfig({});
+ export default defineConfig(forgeViteConfig.preload({}));

@sparecycles
Copy link

I'm a little concerned that this migration guide is implying the de-facto official path uses electron-forge-plugin-vite, which is maintained in a repo under @caoxiemeihao's personal control.

Ideally any migration utility should be contributed and maintained under electron/forge

@caoxiemeihao
Copy link
Member

caoxiemeihao commented Apr 21, 2024

I'm a little concerned that this migration guide is implying the de-facto official path uses electron-forge-plugin-vite, which is maintained in a repo under @caoxiemeihao's personal control.

Ideally any migration utility should be contributed and maintained under electron/forge

v7.3.0 just migrates the previous config in forge/plugin-vite to forge/template-vite. The purpose of this is to upgrade vite, making it very easy to fix some errors.
I even think that plugin-vite/migration itself is not necessary to be released as npm, it is just made for some talents who have difficulty upgrading.
plugin-vite/migration just copies the forge/template-vite config of v7.3.0. Is there any necessity for it to appear in the electron org?

@sparecycles
Copy link

sparecycles commented Apr 21, 2024

Well, migrating any code from forge/plugin-vite to forge/template-vite requires clients to back-port what was migrated into their own templates, and this needs to be documented.

It would be nice if you would explain what was moved, and if possible, please describe the reasons that these were moved (rather than claiming in general "to make it easy to fix some errors" or "very fast release pace of Vite").

Adding another dependency which makes the system an interaction of the versions of forge, vite, the client template, and your migration base doesn't seem like a good idea for anyone involved!

BTW, you've also claimed that these changes enable esm support but as others have pointed out, how these changes enable esm support is not clear. Do you have an example to share with them?

@phlegmaticprogrammer
Copy link

Personally, the way this "migration" is being handled makes me want to run for the hills. I don't think it is acceptable that an upgrade from 7.2 to 7.3 breaks my code, without any warning. The reason I am using something like forge in the first place is because I don't want to look below the abstraction that covers all the messy details.

@GitMurf
Copy link

GitMurf commented Apr 21, 2024

So, I think the expectation here is we recreate our build configs on the new base templates?

@sparecycles in reviewing several of the related gh issues, it seems as if you and I have come to a similar conclusion and have tried similar “workarounds” to get things working for esm/type:module.

Specifically to your quote above, I have been coming to a similar conclusion. It’s a bit of a shame because I have been building and fine tuning our vite-ts-forge config the last year or so and there is a lot in there that I hate to lose / re-implement, but at the same time I think that is part of the issue for me for troubleshooting recent changes, because I have a lot of configurations items that could be throwing things off…. And some are likely no longer necessary.

TLDR: after spending the last couple days trying to “hack” this to work with our current build/ci flows, I think I am going to start from scratch, get esm / type module working (without having to change the format of our import statements etc in our codebase) and then rebuild some of our customization on top of it after the dust settles.

are you thinking similarly? Do you have any public working MVPs is could peak at? Thanks!

@sparecycles
Copy link

@GitMurf unfortunately I'm sort-of developing an in-house project... I'm the sole contributor but I haven't gone through the steps to open-source it and there are a few more features I'd like to build before making it public. (Perhaps I'm being too careful.)

This has gone on long enough that I should probably make a small svelte + vite + electron demo project (with both 7.2.0 and 7.30) to share.

@sparecycles
Copy link

sparecycles commented Apr 21, 2024

Okay! only took about 30 minutes to strip the project down to the vite/svelte/electron/tailwind integration. If you're not a mac user, you may need to add the windows/linux plugins back in (I tend to remove stuff from templates unless I know what they do).

a forge-7.2.0 branch is there
https://github.com/sparecycles/vite-svelte-tailwind-electron

I'll try to do the forge-7.3.0 branch later today and probably have to make a forge-7.4.0 branch if #3572 lands 😝 update: it looks like that PR might not be a breaking change now. yay!

@sparecycles
Copy link

If you manually merge in the new templates, it does work.

https://github.com/sparecycles/vite-svelte-tailwind-electron/compare/forge-7.2.0..forge-7.3.0

It's a little curious though, the three forge configs interact with the "vite.base.config" file differently:

  • the main config uses env.forgeConfigSelf.entry as build.lib.entry
  • the preload config uses env.forgeConfigSelf.entry as build.rollupOptions.input
  • meanwhile the renderer config does not use entry.

Also the renderer config does not mergeConfig(getBuildConfig(), ...) as the other two do (and it's the only one that uses pluginExposeRenderer)

Now it was actually pretty easy at this point to remove my old hack and switch to an esmodule build target!
https://github.com/sparecycles/vite-svelte-tailwind-electron/compare/forge-7.3.0..forge-7.3.0-esm

@GitMurf
Copy link

GitMurf commented Apr 21, 2024

@sparecycles thanks so much for these updates and mvp! On your first one I was going to ask you where / how you were changing from vite cjs to es. But it looks like now in hour update you have that working?

Also, your comments about windows (we build our app for windows and macs) is it really just the makers in your forge config that you stripped out windows stuff? If so that is fine for me because we have some complex package and makers customizations anyways :)

@sparecycles
Copy link

sparecycles commented Apr 21, 2024

@GitMurf it's mostly the makers, I've also attempted to get windows builds working with a non-developer coworker and it hangs (there's issues with native compilation: python 3.12 removed their deprecated distutils package so only node-gyp 10+ supports python 3.12+ and updating node-gyp theoretically needs a one-line change to be supported in electron-rebuild... at least, just patching that change in on mac works, but their machine failed to build and took a long time)... So I guess the makers aren't even needed, I've never gone the extra step of packaging the app into a diskimage or whatever the darwin-specific maker does.

But FWIW, I can't yet support windows issues because I can't test. (started working on spinning up a windows VM this weekend, but things never go smooth!)

I think I can make an alternate config.base.ts that could provide methods to mix-in these new config requirements with less code needing to change (wrap the vite config.ts file's default exports with a function that mixes in the functionality).

Now that I've done the work it's not that bad, but it's annoying for sure to see this framework-type code moved into client-land.

@sparecycles
Copy link

sparecycles commented Apr 22, 2024

@GitMurf Okay, I did the thing.

forge-7.3.0-rewrite-esm-support:./vite.base.config.ts should be a shim that helps people migrate.

Copy-paste that file into the project, and then try taking whatever your existing configs are and wrap them with the appropriate extendMainConfig(), extendPreloadConfig() or extendRendererConfig() wrappers. (these accept all 4 possible forms of configs, objects, functions, promises, and async functions, and will mix in what the vite.base.config and templates currently have).

the only non-trivial (non extendMainConfig({}) and extendPreloadConfig({})) in this example is

export default extendRendererConfig({
  plugins: [svelte({})],
});

because svelte!

I've only tested this with package.json type=module because svelte requires it. If anyone can do the legwork to validate this solution is stable in a commonjs/other-framework package, that would be great.

Thanks for the nudge, I feel like I can finally update my actual project now!

@Ben-Avrahami @boldt this may be easier to apply for you.

EDIT: oof, it appears that I didn't test preload. It seems to be imported by require in electron and therefore needs to be a .cjs file when package.json type=module.

@caoxiemeihao
Copy link
Member

caoxiemeihao commented Apr 22, 2024

Personally, the way this "migration" is being handled makes me want to run for the hills. I don't think it is acceptable that an upgrade from 7.2 to 7.3 breaks my code, without any warning. The reason I am using something like forge in the first place is because I don't want to look below the abstraction that covers all the messy details.

You only see that there is an increase in template code for v7.3.0, and you have overlooked what the approach of v7.3.0 brings and what incompatible aspects of Forge itself have been avoided.

  1. The release speed of Vite is very fast, and Forge will not release a new version immediately due to a version update of Vite.
  2. The modern development experience of hot-restart and hot-reload is very popular in electron-vite and tauri, but implementing this feature in Forge without authorization can damage Forge's own design. It's simply impossible to advance.
  3. It is not possible to directly use {"type": "module"} in the package. json of @electron-forge/plugin-vite, which is fatal for frameworks like svelte, as Forge itself is built in the format of CommonsJS(tsconfig. json) and cannot risk switching to ESNext entirely due to the vite-template.
  4. Vite itself was not designed for Electron and does not even support Node.js construction. Forge exposes the configuration, which is beneficial for users to update the Vite version in a timely manner without waiting for Forge's version update, and it can also more conveniently fix some unknown BUG.

BTW, if you feel that there is too much @electron-forge/template-vite code, it is recommended that you use electron-vite(even without a separate config file, really simplify) or other frameworks.

@sparecycles
Copy link

@caoxiemeihao

Maybe extending the data in forge.config.ts with a flag that allows users to opt into the new template requirements (dropping functionality within forge in favor of providing it in the application) would have been a better route? It would provide the flexibility you're looking for without unpaving the path that everyone else is on. Not everyone needs to be on the latest version of vite not-yet-supported-by-forge!!!

e.g.,

    new VitePlugin({
      baseless: true, // basic configuration will be provided by application instead
      build: [
        {
          entry: "electron/main.ts",
          config: "vite.main.config.ts",
        },
        {
          entry: "electron/preload.ts",
          config: "vite.preload.config.ts",
        },
      ],
      renderer: [
        {
          name: "main_window",
          config: "vite.renderer.config.ts",
        },
      ],
    })

Forge could make that flag the default in a major version release: that's when people expect there to be breaking changes, meanwhile anyone not able to extend their templates is blocked from updating their forge minor version for any other reason!

Maybe the electron/forge maintainers can join this conversation? Are @erickzhao @VerteDinde slackers? (oh yes, yes they are.)

@sparecycles
Copy link

It is not possible to directly use {"type": "module"} in the package.json of @electron-forge/plugin-vite, which is fatal for frameworks like svelte, as Forge itself is built in the format of CommonsJS(tsconfig. json) and cannot risk switching to ESNext entirely due to the vite-template.

it is possible to use {"type": "module"} in the package.json of application. And svelte works fine:
https://github.com/sparecycles/vite-svelte-tailwind-electron/tree/forge-7.2.0-esm (now without the .vite/build/package.json commonjs hack)

@phlegmaticprogrammer
Copy link

phlegmaticprogrammer commented Apr 22, 2024

BTW, if you feel that there is too much Forge template code, it is recommended that you use electron-vite(even without a separate config file, really simplify) or other frameworks.

I don't even know what you mean by "template code" (I guess I will have to find out now), so I don't care if there is much of it or not. All I care about is that I can use TypeScript to write a desktop app, and hot reload is nice (although that feature seems limited, as server code doesn't hot reload, it seems). That's about the level of abstraction I want to think about here. Unfortunately, that approach does not seem viable (it seemed to be, until 7.2 -> 7.3 came along), so this "talent" needs to dig deeper into the mess now.

@GitMurf
Copy link

GitMurf commented Apr 22, 2024

@caoxiemeihao I am fairly well versed in forge and vite but there is so much conversation here (and in other gh issues) about what needs to be patched where and what forge version and running migration scripts etc etc that I am confused and a bit lost.

Is it possible that you publish a minimal working template repo that has everything we need with forge 7.4 to work with forge-vite-typescript and ESM (type: module)? Basically what the vite typescript template should do (but it sounds like there are reasons why the template can't work or something?).

I just think we need some "official" docs / direction that will also be maintained / supported by forge long term otherwise it will turn into a maintenance nightmare. Thanks!

@sparecycles
Copy link

I know the changes I need to get my example repo into a fully working state with forget-7.4.0 (support preload as well), I don't know if anyone is interested in a random person's GLWTSPL solution...

@VerteDinde
Copy link
Member

Hey folks, sorry for our delay - we spoke about this in a maintainer meeting today. We're deciding on a path forward, but wanted to briefly update the issue letting you know that there are currently eyes on this and we should be reporting back with fixes shortly.

@GitMurf
Copy link

GitMurf commented Apr 23, 2024

Ok I think I finally understand the issue. Am I correct that there were changes to the forge vite plugin that required changes to the vite configs? And those changes are part of the forge vite template? But the problem is on existing projects, the templates do not update as it is just starter template code. So for a brand new project, it would work, but for existing projects, you have to manually copy changes that were made to the vite configs that are available in the template?

@MarshallOfSound
Copy link
Member

@GitMurf That is my understanding 👍 As @VerteDinde mentioned above we're looking into options here, the issue is the breaking change is bidirectional. i.e. straight up reverting won't help because then new apps will be broken in the opposite way to old apps. Right now you should pin to a version of forge that works for your app, and we'll figure out how to make a way forward to higher forge versions.

@GitMurf
Copy link

GitMurf commented Apr 23, 2024

That is my understanding 👍

Thank you @MarshallOfSound for your quick response... I feel better that I at least understand the problem now :) if/when you need testers, let me know.

@sparecycles
Copy link

straight up reverting won't help because then new apps will be broken in the opposite way to old apps.

I'm not sure about that. If the missing configuration is left in the template and restored to the plugin, the duplicated configuration should mostly merge back together. 🤞 ❔

noah10 added a commit to EmotionCognitionLab/factorial-design-breathing-study that referenced this issue May 3, 2024
That led to this problem: electron/forge#3506
and this problem: electron/forge#3208
Fixing those led to changes in logger build,
which required updates in downstream code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests