feat(plugin-vite): upgrade to vite@5#3468
Conversation
73ab754 to
b4da787
Compare
3225183 to
6882567
Compare
erickzhao
left a comment
There was a problem hiding this comment.
two quick comments before reviewing the code in-depth
|
CI failed. Can you fix it? |
|
Looks like a Windows only fluke? |
After Vite is built, deletion fails on Windows. |
|
Thank you for the effort you have put into this work. Regarding the failure of unit tests on Windows, my opinion is: "Perhaps we don't need to perform the The reason we need to call In node.js, once you load some So, all you need to do is remove the The purpose of PTAL @erickzhao @erikian |
|
If it helps, I always use |
|
|
||
| (async () => { | ||
| // https://github.com/electron/forge/blob/v7.2.0/packages/utils/test-utils/src/index.ts#L24 | ||
| const dirs = await glob(path.resolve(os.tmpdir(), 'electron-forge-test-*')); |
There was a problem hiding this comment.
macOS CI seems to be failing due to this specific line of code now. If I revert to the previous state (using os.homedir(), the tests pass.
The problem seems to be fixed when I set resolve.preserveSymlinks = true in the renderer config, but I don't know if this is what we want in production.
diff --git a/packages/plugin/vite/src/VitePlugin.ts b/packages/plugin/vite/src/VitePlugin.ts
index 6e71ca7c6..e658bda55 100644
--- a/packages/plugin/vite/src/VitePlugin.ts
+++ b/packages/plugin/vite/src/VitePlugin.ts
@@ -193,6 +193,9 @@ the generated files). Instead, it is ${JSON.stringify(pj.main)}`);
buildRenderer = async (): Promise<void> => {
for (const userConfig of await this.configGenerator.getRendererConfig()) {
await vite.build({
+ resolve: {
+ preserveSymlinks: true,
+ },
configFile: false,
...userConfig,
});There was a problem hiding this comment.
If we put resolve.preserveSymlinks in tmpl/vite.renderer.config.ts, users can remove it at any time.
import { type UserConfig, defineConfig } from 'vite';
import { pluginExposeRenderer } from './vite.base.config';
// https://vitejs.dev/config
export default defineConfig((env) => {
const { root, mode, forgeConfigSelf } = env as ForgeConfigEnv<'renderer'>;
const name = forgeConfigSelf.name ?? '';
return {
root,
mode,
base: './',
build: {
outDir: `.vite/renderer/${name}`,
},
plugins: [pluginExposeRenderer(name)],
+ resolve: {
+ preserveSymlinks: true,
+ },
clearScreen: false,
} as UserConfig;
});There was a problem hiding this comment.
It seems to be caused by fast-glob? Maybe you can use os.readdir(os.tmpdir()); instead?
There was a problem hiding this comment.
Note that this option can break the Vite dev server when the Electron Forge project is part of a monorepo with workspaces (e.g. Yarn workspaces, Turborepo etc.). This seems to happen because workspaces work by adding symlinks to each workspace's source folder inside node_modules, so messing with symlink resolution means the dependency pre-bundling step can no longer tell that files which are part of other workspaces in the monorepo are actually source files instead of dependencies. As a result, the CommonJS/UMD compatibility step breaks, and users can end up with weird "the requested module does not provide an export named default" errors.
And since this preserveSymlinks: true option is now "hidden" as part of the base vite.renderer.config.ts instead of being part of the user's project, it took me a while to diagnose the error.
1d719bf to
d47625e
Compare
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@electron-forge/maker-base](https://togithub.com/electron/forge) | [`7.2.0` -> `7.3.0`](https://renovatebot.com/diffs/npm/@electron-forge%2fmaker-base/7.2.0/7.3.0) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | | [@napi-rs/cli](https://togithub.com/napi-rs/napi-rs) | [`3.0.0-alpha.40` -> `3.0.0-alpha.41`](https://renovatebot.com/diffs/npm/@napi-rs%2fcli/3.0.0-alpha.40/3.0.0-alpha.41) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>electron/forge (@​electron-forge/maker-base)</summary> ### [`v7.3.0`](https://togithub.com/electron/forge/releases/tag/v7.3.0) [Compare Source](https://togithub.com/electron/forge/compare/v7.2.0...v7.3.0) ##### What's Changed ##### Features - feat(plugin-vite): upgrade to vite@5 by [@​caoxiemeihao](https://togithub.com/caoxiemeihao) in [https://github.com/electron/forge/pull/3468](https://togithub.com/electron/forge/pull/3468) - feat: allow a custom out dir from forge config by [@​lutzroeder](https://togithub.com/lutzroeder) in [https://github.com/electron/forge/pull/3458](https://togithub.com/electron/forge/pull/3458) - feat(template-vite): patch types by [@​caoxiemeihao](https://togithub.com/caoxiemeihao) in [https://github.com/electron/forge/pull/3494](https://togithub.com/electron/forge/pull/3494) - feat: adds default fuses to templates by [@​yangannyx](https://togithub.com/yangannyx) in [https://github.com/electron/forge/pull/3480](https://togithub.com/electron/forge/pull/3480) - feat(publisher-github): option to automatically generate release notes by [@​dsanders11](https://togithub.com/dsanders11) in [https://github.com/electron/forge/pull/3484](https://togithub.com/electron/forge/pull/3484) ##### Fixes - fix(electron-release-publisher): change api/version endpoint in PublisherERS to use versions/sorted by [@​kgallagher52](https://togithub.com/kgallagher52) in [https://github.com/electron/forge/pull/3431](https://togithub.com/electron/forge/pull/3431) - fix(core): packageJSON won't be found when programmatic usage instead of CLI by [@​ianho](https://togithub.com/ianho) in [https://github.com/electron/forge/pull/3455](https://togithub.com/electron/forge/pull/3455) - fix: actually depend on preceeding groups by [@​MarshallOfSound](https://togithub.com/MarshallOfSound) in [https://github.com/electron/forge/pull/3438](https://togithub.com/electron/forge/pull/3438) - fix: normalize windows version with build part correctly by [@​rickymohk](https://togithub.com/rickymohk) in [https://github.com/electron/forge/pull/3461](https://togithub.com/electron/forge/pull/3461) - fix: .vscode settings.json changes on open by [@​lutzroeder](https://togithub.com/lutzroeder) in [https://github.com/electron/forge/pull/3460](https://togithub.com/electron/forge/pull/3460) - fix(plugin-vite): package volume size to large by [@​caoxiemeihao](https://togithub.com/caoxiemeihao) in [https://github.com/electron/forge/pull/3336](https://togithub.com/electron/forge/pull/3336) ##### Performance - refactor: only run webpack once for multi-arch packages by [@​MarshallOfSound](https://togithub.com/MarshallOfSound) in [https://github.com/electron/forge/pull/3437](https://togithub.com/electron/forge/pull/3437) ##### Other Changes - chore: update Packager by [@​erikian](https://togithub.com/erikian) in [https://github.com/electron/forge/pull/3419](https://togithub.com/electron/forge/pull/3419) - chore: bump electronjs/node to 2.2.0 (main) by [@​electron-roller](https://togithub.com/electron-roller) in [https://github.com/electron/forge/pull/3469](https://togithub.com/electron/forge/pull/3469) - chore(plugins/electronegativity): correct some config types by [@​Dogdriip](https://togithub.com/Dogdriip) in [https://github.com/electron/forge/pull/3482](https://togithub.com/electron/forge/pull/3482) - chore: use Dependabot to update GitHub Actions deps by [@​dsanders11](https://togithub.com/dsanders11) in [https://github.com/electron/forge/pull/3487](https://togithub.com/electron/forge/pull/3487) - chore: bump electronjs/node to 2.2.1 (main) by [@​electron-roller](https://togithub.com/electron-roller) in [https://github.com/electron/forge/pull/3496](https://togithub.com/electron/forge/pull/3496) ##### New Contributors - [@​kgallagher52](https://togithub.com/kgallagher52) made their first contribution in [https://github.com/electron/forge/pull/3431](https://togithub.com/electron/forge/pull/3431) - [@​rickymohk](https://togithub.com/rickymohk) made their first contribution in [https://github.com/electron/forge/pull/3461](https://togithub.com/electron/forge/pull/3461) - [@​lutzroeder](https://togithub.com/lutzroeder) made their first contribution in [https://github.com/electron/forge/pull/3460](https://togithub.com/electron/forge/pull/3460) - [@​ianho](https://togithub.com/ianho) made their first contribution in [https://github.com/electron/forge/pull/3455](https://togithub.com/electron/forge/pull/3455) - [@​yangannyx](https://togithub.com/yangannyx) made their first contribution in [https://github.com/electron/forge/pull/3480](https://togithub.com/electron/forge/pull/3480) - [@​Dogdriip](https://togithub.com/Dogdriip) made their first contribution in [https://github.com/electron/forge/pull/3482](https://togithub.com/electron/forge/pull/3482) **Full Changelog**: electron/forge@v7.2.0...v7.3.0 </details> <details> <summary>napi-rs/napi-rs (@​napi-rs/cli)</summary> ### [`v3.0.0-alpha.41`](https://togithub.com/napi-rs/napi-rs/releases/tag/%40napi-rs/cli%403.0.0-alpha.41) [Compare Source](https://togithub.com/napi-rs/napi-rs/compare/@napi-rs/cli@3.0.0-alpha.40...@napi-rs/cli@3.0.0-alpha.41) ##### What's Changed - fix(cli): fallback to wasm32 if platform is not support by [@​Brooooooklyn](https://togithub.com/Brooooooklyn) in [https://github.com/napi-rs/napi-rs/pull/1967](https://togithub.com/napi-rs/napi-rs/pull/1967) - fix(cli): allow more platform & arch fallback to wasm by [@​Brooooooklyn](https://togithub.com/Brooooooklyn) in [https://github.com/napi-rs/napi-rs/pull/1969](https://togithub.com/napi-rs/napi-rs/pull/1969) **Full Changelog**: https://github.com/napi-rs/napi-rs/compare/napi@2.15.3...[@​napi-rs/cli](https://togithub.com/napi-rs/cli)[@​3](https://togithub.com/3).0.0-alpha.41 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/toeverything/AFFiNE). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMDAuMCIsInVwZGF0ZWRJblZlciI6IjM3LjIwMC4wIiwidGFyZ2V0QnJhbmNoIjoiY2FuYXJ5In0=-->
|
There were a lot of linked gh issues about ESM support but was this PR supposed to address ESM? I still cannot get the vite-typescript plugin to work with ESM type: module. In this comment @caoxiemeihao closed the issue related to ESM linking to this PR but it does not seem like it was actually related to that, was it? #3439 (comment) I apologize as I am very confused but I have tried everything I know how to get it to work and to track down the status of ESM for vite-typescript but keep hitting dead ends. Thanks in advance for help/updates! |


Summarize your changes:
Fixes:
#3309
#3423
#3439
#3466
Breaks:
Drawing lessons form the bugs encountered, this PR change is very significant. Moving most of the logic form
plugin/vitetotemplate/vitewill bring the following benefits.vite.config.mjsin the template(local project).