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

feat(plugin-vite): upgrade to vite@5 #3468

Merged
merged 19 commits into from
Feb 7, 2024

Conversation

caoxiemeihao
Copy link
Member

@caoxiemeihao caoxiemeihao commented Jan 18, 2024

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

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/vite to template/vite will bring the following benefits.

  1. Adapt to the very fast release pace of Vite, and users can modify the version of Vite in the template at any time.
  2. If users encounter some problems, users can solve them by directly modifying the vite.config.mjs in the template(local project).
  3. More transparent and flexible Vite config.
  4. It is easy to implement hot-restart and hot-reload 🔥

@caoxiemeihao caoxiemeihao requested a review from a team as a code owner January 18, 2024 05:57
@caoxiemeihao caoxiemeihao force-pushed the feat/upgrade-vite5 branch 3 times, most recently from 73ab754 to b4da787 Compare January 19, 2024 06:21
@caoxiemeihao caoxiemeihao changed the title WIP(feat(plugin-vite)): upgrade to vite@5 feat(plugin-vite): upgrade to vite@5 Jan 22, 2024
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two quick comments before reviewing the code in-depth

package.json Show resolved Hide resolved
packages/plugin/vite/src/Config.ts Show resolved Hide resolved
@BlackHole1
Copy link
Member

CI failed. Can you fix it?

@Stanzilla
Copy link

Looks like a Windows only fluke?

@caoxiemeihao
Copy link
Member Author

Looks like a Windows only fluke?

After Vite is built, deletion fails on Windows.

@erickzhao erickzhao self-requested a review January 31, 2024 01:16
@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Feb 1, 2024

About CI failed on Windows 😅

It seems that Windows platforms have resource occupancy issues when using Native addons. Previously, esbuild.exe occupied file resources, but now it has become rollup@4+(vite@5+).

This is a headache and I don't know how to end the rollup.win32-x64-msvc.node resource usage problem.
The problems currently encountered are as follows:

  • kill esbuild.exe
  • kill rollup.win32-x64-msvc.node ❌. Can anyone provide me with some help? 🤔

The error occur here 👉 image


esbuild.exe native addons error (resolved ✅)

image
Error: EBUSY: resource busy or locked, rmdir '\\?\C:\Users\Administrator\Desktop\forge-test-vite-2'
    at rmdirSync (node:fs:1222:10)
    at _rmdirSync (node:internal/fs/rimraf:235:5)
    at rimrafSync (node:internal/fs/rimraf:193:7)
    at Object.rmSync (node:fs:1271:10)
    at Timeout.remove [as _onTimeout] (D:\cxmh\electron-forge-vite-large\packages\template\vite-typescript\test\ViteTypeScriptTemplate_spec_slow.ts:33:12)
    at listOnTimeout (node:internal/timers:569:17)
    at processTimers (node:internal/timers:512:7) {
  errno: -4082,
  syscall: 'rmdir',
  code: 'EBUSY',
  path: '\\\\?\\C:\\Users\\Administrator\\Desktop\\forge-test-vite-2'
}

rollup native addons error (not resolved yet ❌)

Error: EPERM: operation not permitted, unlink '\\?\C:\Users\Administrator\Desktop\forge-test-vite-2\node_modules\@
    at unlinkSync (node:fs:1881:3)
    at _unlinkSync (node:internal/fs/rimraf:214:14)
    at fixWinEPERMSync (node:internal/fs/rimraf:306:5)
    at rimrafSync (node:internal/fs/rimraf:200:14)
    at node:internal/fs/rimraf:253:9
    at Array.forEach (<anonymous>)
    at _rmdirSync (node:internal/fs/rimraf:250:7)
    at fixWinEPERMSync (node:internal/fs/rimraf:304:5)
    at rimrafSync (node:internal/fs/rimraf:200:14)
    at node:internal/fs/rimraf:253:9
    at Array.forEach (<anonymous>)
    at _rmdirSync (node:internal/fs/rimraf:250:7)
    at fixWinEPERMSync (node:internal/fs/rimraf:304:5)
    at rimrafSync (node:internal/fs/rimraf:200:14)
    at node:internal/fs/rimraf:253:9
    at Array.forEach (<anonymous>)
    at _rmdirSync (node:internal/fs/rimraf:250:7)
    at fixWinEPERMSync (node:internal/fs/rimraf:304:5)
    at rimrafSync (node:internal/fs/rimraf:200:14)
    at node:internal/fs/rimraf:253:9
    at Array.forEach (<anonymous>)
    at _rmdirSync (node:internal/fs/rimraf:250:7)
    at fixWinEPERMSync (node:internal/fs/rimraf:304:5)
    at rimrafSync (node:internal/fs/rimraf:200:14)
    at Object.rmSync (node:fs:1271:10)
    at Timeout.remove [as _onTimeout] (D:\cxmh\electron-forge-vite-large\packages\template\vite-typescript\test\Vi
    at listOnTimeout (node:internal/timers:569:17)
    at processTimers (node:internal/timers:512:7) {
  errno: -4048,
  syscall: 'unlink',
  code: 'EPERM',
  path: '\\\\?\\C:\\Users\\Administrator\\Desktop\\forge-test-vite-2\\node_modules\\@rollup\\rollup-win32-x64-msvc
}

@BlackHole1
Copy link
Member

BlackHole1 commented Feb 1, 2024

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 fs.rmSync operation here."

The reason we need to call fs.rmSync is to avoid excessive disk usage, as Windows does not automatically delete TMPDIR.

In node.js, once you load some node addon, you will find that you cannot reset it. Node.js will hold onto it continuously due to this feature, combined with the file lock in Windows (a file handle can only be held by one process). This is why you encountered this issue.

So, all you need to do is remove the fs.rmSync method here and modify the test script in package.json to something like this: "test": "npm run clear:test && npm run test:slow".

The purpose of test:clear is to delete all folders under os.tmpdir() that start with electron-forge-test-.

PTAL @erickzhao @erikian

@Stanzilla
Copy link

If it helps, I always use git clean -xdf instead of cleanup scripts for files that are not checked in. You can also specify a directory to only delete one like git clean -xdf tests


(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-*'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
       });

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
});

Copy link
Member

@BlackHole1 BlackHole1 Feb 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be caused by fast-glob? Maybe you can use os.readdir(os.tmpdir()); instead?

@VerteDinde VerteDinde merged commit b0cae41 into electron:main Feb 7, 2024
7 checks passed
Brooooooklyn referenced this pull request in toeverything/AFFiNE Feb 23, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](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) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@electron-forge%2fmaker-base/7.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@electron-forge%2fmaker-base/7.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@electron-forge%2fmaker-base/7.2.0/7.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@electron-forge%2fmaker-base/7.2.0/7.3.0?slim=true)](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) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@napi-rs%2fcli/3.0.0-alpha.41?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@napi-rs%2fcli/3.0.0-alpha.41?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@napi-rs%2fcli/3.0.0-alpha.40/3.0.0-alpha.41?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@napi-rs%2fcli/3.0.0-alpha.40/3.0.0-alpha.41?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>electron/forge (@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;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 [@&#8203;electron-roller](https://togithub.com/electron-roller) in [https://github.com/electron/forge/pull/3496](https://togithub.com/electron/forge/pull/3496)

##### New Contributors

-   [@&#8203;kgallagher52](https://togithub.com/kgallagher52) made their first contribution in [https://github.com/electron/forge/pull/3431](https://togithub.com/electron/forge/pull/3431)
-   [@&#8203;rickymohk](https://togithub.com/rickymohk) made their first contribution in [https://github.com/electron/forge/pull/3461](https://togithub.com/electron/forge/pull/3461)
-   [@&#8203;lutzroeder](https://togithub.com/lutzroeder) made their first contribution in [https://github.com/electron/forge/pull/3460](https://togithub.com/electron/forge/pull/3460)
-   [@&#8203;ianho](https://togithub.com/ianho) made their first contribution in [https://github.com/electron/forge/pull/3455](https://togithub.com/electron/forge/pull/3455)
-   [@&#8203;yangannyx](https://togithub.com/yangannyx) made their first contribution in [https://github.com/electron/forge/pull/3480](https://togithub.com/electron/forge/pull/3480)
-   [@&#8203;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 (@&#8203;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 [@&#8203;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 [@&#8203;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...[@&#8203;napi-rs/cli](https://togithub.com/napi-rs/cli)[@&#8203;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=-->
@GitMurf
Copy link

GitMurf commented Apr 19, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants