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

fix(plugin-vite): package volume size to large #3336

Merged
merged 19 commits into from
Jan 17, 2024

Conversation

caoxiemeihao
Copy link
Member

@caoxiemeihao caoxiemeihao commented Sep 4, 2023

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

#3224
#3298
#3293
#3377
#3420

At present, I still need to exactly include the third-party packages in dependencies in to node_modules, because they cannot be built normally be Vite, such as C/C++ addons.
I haven't found the right logic to handle node_modules, I'm trying...

@caoxiemeihao caoxiemeihao requested a review from a team as a code owner September 4, 2023 04:53
@caoxiemeihao caoxiemeihao changed the title wip(fix(vite)): package size volume to large WIP(fix(plugin-vite)): package volume size to large Sep 4, 2023
@BlackHole1 BlackHole1 marked this pull request as draft September 4, 2023 09:14
@PascalPixel PascalPixel mentioned this pull request Oct 8, 2023
3 tasks
@GitMurf
Copy link

GitMurf commented Nov 9, 2023

Is there any update on this? Saw a few people recently post about this issue in the Electron Discord server and pointed them here for reference.

@GitMurf
Copy link

GitMurf commented Nov 9, 2023

At present, I still need to exactly include the third-party packages in dependencies in to node_modules, because they cannot be built normally be Vite, such as C/C++ addons. I haven't found the right logic to handle node_modules, I'm trying...

@caoxiemeihao Have you looked at the code I posted here (#3224 (comment))? I believe this is the type of solution you are looking for. Whatever node_modules vite has to externalize, we need to whitelist to include in the package. The tricky part is we then need to also include its nested dependencies. This is where I used the walker created by @MarshallOfSound called flora-colossus.

@xhc-code
Copy link

Why not use vite to build the dist directory and use the built html resources in the dist directory? Now the behavior of plugin-vite is to copy the entire source code (including the directory that node_modules depends on) to the out directory, and then perform the build operation again. My project built with Vite is only 108kb, but built with this plug-in it is nearly 700m. I don’t know what I did wrong.

@an-lee
Copy link

an-lee commented Nov 24, 2023

Hello! I hope you're doing well. Any chance of an update? We're eagerly waiting to release a new app with vite. Your contributions are greatly appreciated! Thank you!

@BlackHole1
Copy link
Member

Ping @caoxiemeihao

@akash07k
Copy link

akash07k commented Dec 9, 2023

@caoxiemeihao Is there any update to this?
I hope this gets merged soon.

@sipt
Copy link

sipt commented Dec 20, 2023

I already raised this issue last year. #3071 (comment)

I resolved by modifying the forge.config.ts file.

const config: ForgeConfig = {
  packagerConfig: {
    ignore: (file) => {
      if (!file) return false;
      return (
        !/^[/\\]package.json$/.test(file) &&
        !/^[/\\]\.vite($|[/\\]).*$/.test(file)
      );
    },
  },
}

@GitMurf
Copy link

GitMurf commented Dec 20, 2023

I already raised this issue last year. #3071 (comment)

I resolved by modifying the forge.config.ts file...

I have a similar solution but the problem is for a generic solution it is more complicated because if you have any native node libs (like Realm or SQLite etc) you have to add more logic to include them since they do not get bundled (external). And then you have to check their nested dependencies and also include them. I had to do quite a bit of extra logic to get it working for us and I think that is the complication here, that they need to come up with a way to generically handle this for all users.

@caoxiemeihao caoxiemeihao force-pushed the fix/vite-package-volume branch from f7ba5d7 to 7560f15 Compare December 31, 2023 00:43
@caoxiemeihao
Copy link
Member Author

I'm very sorry I've been busy with my work and I'm currently trying to fix it.

@caoxiemeihao caoxiemeihao force-pushed the fix/vite-package-volume branch from 7560f15 to 86c0745 Compare December 31, 2023 08:23
@caoxiemeihao caoxiemeihao changed the title WIP(fix(plugin-vite)): package volume size to large fix(plugin-vite): package volume size to large Dec 31, 2023
@caoxiemeihao caoxiemeihao marked this pull request as ready for review December 31, 2023 08:24
@caoxiemeihao caoxiemeihao force-pushed the fix/vite-package-volume branch from 86c0745 to c67f0f6 Compare December 31, 2023 14:17
@caoxiemeihao caoxiemeihao force-pushed the fix/vite-package-volume branch from e2f9bec to 51f5aec Compare January 1, 2024 03:51
@caoxiemeihao caoxiemeihao force-pushed the fix/vite-package-volume branch from 65eea3e to 2f6602f Compare January 8, 2024 12:09
@caoxiemeihao caoxiemeihao force-pushed the fix/vite-package-volume branch 2 times, most recently from 04253d4 to 0564e07 Compare January 17, 2024 11:33
@PascalPixel
Copy link

Awesome work!

@sethyuan
Copy link

This is not yet released, right? I'm using "@electron-forge/plugin-vite": "^7.2.0" and it still bundles the whole src.
When is a release going to be available?

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://github.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://github.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://github.com/electron/forge/releases/tag/v7.3.0)

[Compare Source](https://github.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://github.com/caoxiemeihao) in [https://github.com/electron/forge/pull/3468](https://github.com/electron/forge/pull/3468)
-   feat: allow a custom out dir from forge config by [@&#8203;lutzroeder](https://github.com/lutzroeder) in [https://github.com/electron/forge/pull/3458](https://github.com/electron/forge/pull/3458)
-   feat(template-vite): patch types by [@&#8203;caoxiemeihao](https://github.com/caoxiemeihao) in [https://github.com/electron/forge/pull/3494](https://github.com/electron/forge/pull/3494)
-   feat: adds default fuses to templates by [@&#8203;yangannyx](https://github.com/yangannyx) in [https://github.com/electron/forge/pull/3480](https://github.com/electron/forge/pull/3480)
-   feat(publisher-github): option to automatically generate release notes by [@&#8203;dsanders11](https://github.com/dsanders11) in [https://github.com/electron/forge/pull/3484](https://github.com/electron/forge/pull/3484)

##### Fixes

-   fix(electron-release-publisher): change api/version endpoint in PublisherERS to use versions/sorted by [@&#8203;kgallagher52](https://github.com/kgallagher52) in [https://github.com/electron/forge/pull/3431](https://github.com/electron/forge/pull/3431)
-   fix(core): packageJSON won't be found when programmatic usage instead of CLI by [@&#8203;ianho](https://github.com/ianho) in [https://github.com/electron/forge/pull/3455](https://github.com/electron/forge/pull/3455)
-   fix: actually depend on preceeding groups by [@&#8203;MarshallOfSound](https://github.com/MarshallOfSound) in [https://github.com/electron/forge/pull/3438](https://github.com/electron/forge/pull/3438)
-   fix: normalize windows version with build part correctly by [@&#8203;rickymohk](https://github.com/rickymohk) in [https://github.com/electron/forge/pull/3461](https://github.com/electron/forge/pull/3461)
-   fix: .vscode settings.json changes on open by [@&#8203;lutzroeder](https://github.com/lutzroeder) in [https://github.com/electron/forge/pull/3460](https://github.com/electron/forge/pull/3460)
-   fix(plugin-vite): package volume size to large by [@&#8203;caoxiemeihao](https://github.com/caoxiemeihao) in [https://github.com/electron/forge/pull/3336](https://github.com/electron/forge/pull/3336)

##### Performance

-   refactor: only run webpack once for multi-arch packages by [@&#8203;MarshallOfSound](https://github.com/MarshallOfSound) in [https://github.com/electron/forge/pull/3437](https://github.com/electron/forge/pull/3437)

##### Other Changes

-   chore: update Packager by [@&#8203;erikian](https://github.com/erikian) in [https://github.com/electron/forge/pull/3419](https://github.com/electron/forge/pull/3419)
-   chore: bump electronjs/node to 2.2.0 (main) by [@&#8203;electron-roller](https://github.com/electron-roller) in [https://github.com/electron/forge/pull/3469](https://github.com/electron/forge/pull/3469)
-   chore(plugins/electronegativity): correct some config types  by [@&#8203;Dogdriip](https://github.com/Dogdriip) in [https://github.com/electron/forge/pull/3482](https://github.com/electron/forge/pull/3482)
-   chore: use Dependabot to update GitHub Actions deps by [@&#8203;dsanders11](https://github.com/dsanders11) in [https://github.com/electron/forge/pull/3487](https://github.com/electron/forge/pull/3487)
-   chore: bump electronjs/node to 2.2.1 (main) by [@&#8203;electron-roller](https://github.com/electron-roller) in [https://github.com/electron/forge/pull/3496](https://github.com/electron/forge/pull/3496)

##### New Contributors

-   [@&#8203;kgallagher52](https://github.com/kgallagher52) made their first contribution in [https://github.com/electron/forge/pull/3431](https://github.com/electron/forge/pull/3431)
-   [@&#8203;rickymohk](https://github.com/rickymohk) made their first contribution in [https://github.com/electron/forge/pull/3461](https://github.com/electron/forge/pull/3461)
-   [@&#8203;lutzroeder](https://github.com/lutzroeder) made their first contribution in [https://github.com/electron/forge/pull/3460](https://github.com/electron/forge/pull/3460)
-   [@&#8203;ianho](https://github.com/ianho) made their first contribution in [https://github.com/electron/forge/pull/3455](https://github.com/electron/forge/pull/3455)
-   [@&#8203;yangannyx](https://github.com/yangannyx) made their first contribution in [https://github.com/electron/forge/pull/3480](https://github.com/electron/forge/pull/3480)
-   [@&#8203;Dogdriip](https://github.com/Dogdriip) made their first contribution in [https://github.com/electron/forge/pull/3482](https://github.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://github.com/napi-rs/napi-rs/releases/tag/%40napi-rs/cli%403.0.0-alpha.41)

[Compare Source](https://github.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://github.com/Brooooooklyn) in [https://github.com/napi-rs/napi-rs/pull/1967](https://github.com/napi-rs/napi-rs/pull/1967)
-   fix(cli): allow more platform & arch fallback to wasm by [@&#8203;Brooooooklyn](https://github.com/Brooooooklyn) in [https://github.com/napi-rs/napi-rs/pull/1969](https://github.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://github.com/napi-rs/cli)[@&#8203;3](https://github.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://github.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 24, 2024

Looking back at this, isn't this just marking all dependencies from the package.json as "External" for vite and then copying all dependencies from package.json into the final package in the packageAfterCopy forge hook?

So doesn't this still copy the entire node_modules folder and prevent doing any tree shaking with vite (since all packages are marked as external)?

@caoxiemeihao
Copy link
Member Author

Looking back at this, isn't this just marking all dependencies from the package.json as "External" for vite and then copying all dependencies from package.json into the final package in the packageAfterCopy forge hook?

So doesn't this still copy the entire node_modules folder and prevent doing any tree shaking with vite (since all packages are marked as external)?

Only dependencies in dependencies will be copied. The dependencies in devDependencies will not be copied, they will be built by Vite.
I hope you can carefully test this obvious difference.
Even if you use electron-builder you will get the same effect.

@GitMurf
Copy link

GitMurf commented Apr 25, 2024

@caoxiemeihao I am just here trying to understand and to help where I can (as seen by several of my comments above in this PR sharing some of my solutions I tried). If my last comment came off any differently I apologize... we are all on the same team 👍

As per your response. Dev dependencies don't build at all as they are part of the dev process. Vite doesn't bundle them unless you mark a true dependency as dev accidentally. It should only be stuff like forge, eslint, prettier, vite etc etc.

Vite has the ability to mark packages as external primarily for things like native node addons which need to be packaged with the app (realm and SQLite in my case). But a majority of dependencies are supposed to be bundled with the rest of the code and tree shaken (if desired) leaving only those external packages needing to be packaged from node modules with the electron app.

As an example, my build code goes from 250mb (with the current vite plugin from this PR which copies all node modules marked as dependency)... down to 29mb when I only include the external native node addons and allow all my other dependencies to be tree shaken and bundled by vite (which they already do anyways).

@MarshallOfSound given you wrote flora-collosus maybe you could chime in here with your 2 cents and correct me if I am mistaken? Or can provide any guidance? Thanks!

@GitMurf
Copy link

GitMurf commented Apr 25, 2024

Here is my "hack" I applied to the forge-plugin-vite util/package.js file with a pnpm patch to override the current functionality so that it only packages my 4 external libs (along with all their nested dependencies). This ties to when I referenced in my last message going from 250mb down to 29mb... and to potentially use as a MVP / idea for implementing. My initial thought would be allow this to read from your vite config the libs you mark as External and only package those ones (as the rest are built/bundled by vite with your main.js etc.).

In my vite config I also set these same 4 packages as External, as opposed to all Dependencies from package.json as the forge vite typescript template defaults to.

Hope this helps.

Patch name: @electron-forge__plugin-vite@7.4.0.patch

diff --git a/dist/util/package.js b/dist/util/package.js
index be2a9f9b575fb0d9eb2d7eb4df9e500e3baf26b0..f5c42c1dc7f8b0b6d712b3f4dc2be9fd93e28c7e 100644
--- a/dist/util/package.js
+++ b/dist/util/package.js
@@ -34,7 +34,15 @@ async function lookupNodeModulesPaths(root, paths = []) {
 }
 exports.lookupNodeModulesPaths = lookupNodeModulesPaths;
 async function resolveDependencies(root) {
-    const rootDependencies = Object.keys((await fs_extra_1.default.readJson(node_path_1.default.join(root, 'package.json'))).dependencies || {});
+    // FIXME: spm patch to allow tree shaking with vite and forge
+    //
+    // const rootDependencies = Object.keys((await fs_extra_1.default.readJson(node_path_1.default.join(root, 'package.json'))).dependencies || {});
+    const rootDependencies = [
+      'realm',
+      'electron-squirrel-startup',
+      'better-sqlite3',
+      'mdast-util-from-markdown',
+    ];
     const resolve = async (prePath, dependencies, collected = new Map()) => await Promise.all(dependencies.map(async (name) => {
         let curPath = prePath, depPath = null, packageJson = null;
         while (!packageJson && !isRootPath(curPath)) {

@joeyballentine
Copy link

I'm finding the same thing by the way (on the latest forge version, 7.4.0). Vite bundles ~200mb of my node modules, which not only increases the total bundle size, but also increases packing time and extraction time.

Using the webpack plugin, exactly 0 node modules are bundled. That's right, nothing. Why is vite so different here? Shouldn't it be tree shaking all these?

@joeyballentine
Copy link

Using the patch mentioned above, Vite is now properly bundling everything, including proper tree shaking.

@caoxiemeihao

This comment was marked as outdated.

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Apr 26, 2024

@MarshallOfSound given you wrote flora-collosus maybe you could chime in here with your 2 cents and correct me if I am mistaken? Or can provide any guidance? Thanks!

Sorry, I didn't discover flora-collosus before, if necessary I will submit a PR and switch to flora-dollosus.

@joeyballentine
Copy link

Using the patch mentioned above, Vite is now properly bundling everything, including proper tree shaking.

Based on community feedback. I updated the API design for migration to 7.3.0+, see #3506 (comment).

That has nothing to do with this issue or the patch. My project is new and created with the newer template. Do you actually understand what the issue is here?

@MrgSub
Copy link

MrgSub commented Oct 16, 2024

This issue started happening randomly in my project. Setting external packages to a few modules makes the build pass but no modules are bundled in the resultant .js file.

@GitMurf
Copy link

GitMurf commented Oct 16, 2024

This issue started happening randomly in my project. Setting external packages to a few modules makes the build pass but no modules are bundled in the resultant .js file.

"No modules" are bundled? or you mean just the external ones are not bundled? Because the external ones are not meant to be bundled in the .js file.

@MrgSub
Copy link

MrgSub commented Oct 16, 2024

The issue is happening due to my project using 7.5.0 instead of 7.4.0
Previous behavior was including all of my npm modules into the published package, now when I start my app it throws an error {x} module not found for each npm package my app is relying on.
Edit: The above worked for me, I was able to have all the node modules needed to run the app in the bundle. The next issue was that electron-squirrel-startup was also marked as external, it needed to be excluded. The next issue was due to sentry, I took it off. finally I have a build that runs.

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

Successfully merging this pull request may close these issues.