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

build: use vite to bundle deps at main/preload package to avoid using broken electron-builder deps function. #903

Closed
wants to merge 5 commits into from

Conversation

Tanimodori
Copy link
Contributor

@Tanimodori Tanimodori commented Feb 8, 2023

Main purpose of this PR

Currently deps used at main/preload package is put under dependencies under package.json and building of those packages is using vite's ssr mode, which means deps will not be handled by vite but electron-builder instead. This PR will use vite to bundle deps at main/preload package just as the same as the renderer package to avoid using broken electron-builder deps function.

Why switching how deps get bundled?

Because deps bundle function is broken in electron-builder:

You need to maintain good old npm node_modules structure.

electron-builder internally uses develar/app-builder whose node_modules bundling function (nodeModuleCollector.go) only recognize good old npm node_modules structure.

It doesn't recognize pnpm's soft-linking node_modules structure. So if your deps used in main/preload have their own sub-dependencies, it will NOT get copied by electron-builder resulting in runtime error because of missing deps (electron-userland/electron-builder#6289). The workaround currently is to tell pnpm using hoist deps mode which is basically to force pnpm using good old npm node_modules structure and forgets about soft-linking packages. It's painful if you are using pnpm in monorepos to avoid files copying.

I don't use yarn but I suppose yarn's PnP structure will also been rejected. (electron-userland/electron-builder#6639)

For monorepos, it has to maintain the same structure. This has been reported in electron-userland/electron-builder#6933 , electron-userland/electron-builder#7371 .

Those issues don't seem to be addressed by the maintainers.

Poor deps bundling

The deps bundling by electron-builder is poorly done:

  • Only *.md and *.d.ts gets removed. Other files (e.g. sourcemaps, artifacts of different archs) will be copied as-is.
  • No vite's tree-shaking. Unused code chunks, cjs+esm codes will be bundled.
  • No minify at all. Code in deps will be copied as-is with no minify.
  • Optional devdeps will be bundled. For example rxjs which is optional devdep of typed-emitter, which is dep of electron-updater, will be bundled. The dep search strategy of electron-builder is just "grab all deps and optional deps" regardless of whether a optional dep is devdep.

In my opinion, this is done poorly compared to vite/esbuild. It will increase the whole production build size which electron apps are often criticized for.

How this PR is going to fix it?

  1. Use builtin-modules and electron-builtins to determine node and electron builtin modules.
  2. Use build.rollupOptions.external to externalize those modules. Optionally you can use vite-plugin-commonjs-externals if any cjs-esm interop issues is met.
      external: src => {
        const [name] = src.split('/');
        const externalNames = [
          ...nodeBuiltins,
          ...nodeBuiltins.map(name => `node:${name}`),
          ...electronBuiltins,
        ];
        return externalNames.includes(name);
      },
  1. Forbid electron-builder from copying any node_module files as now all deps are bundled by vite.
  2. Move vue to dependencies under package.json (revert move vue to dev dependencies #901). Any further deps used by all three packages can also be added for distinguish which will be ships to builds clearly.

Have this PR been tested?

I've tested the template and my own rush + pnpm monorepo setup on Windows 10 which is flawless. Both watch and production build work as intended.

I want to share package/code between main/preload package

For packages, if it's written in ESM and the code used in main/preload is from different chunk, it will be fine due to vite's tree-shaking feature. Otherwise you can transform this project's structure to utilize vite's multi-entry feature to achieve the goal of reusing codes.

@cawa-93
Copy link
Owner

cawa-93 commented Feb 8, 2023

Hi there. Thanks for you contrib. Basically you propose a kinda revert d002d6f.

From another issue

Previously it works exacly as you described. But this led to numerous problems (#664, #655, #613), because there are a huge number of packages that he will not be able to fully bundle. Usually these are packages that depend on native APIs directly or transitively.

As example: axios.
In your case you can't use axios in main anymore. AFAIR vite will resolved axios to browser-specific XHR adapter if ssr: false. To solve this, you forced to add axios to external packages. If so, you also forced to include full node_modules into compiled app. And I didn't found any way to do that partially.

Also I remember some issues with electron-updater since it use semver that not bundling properly.

@Tanimodori
Copy link
Contributor Author

Thanks for your quick reply and the summary. It seems I've totally missed the point that ssr is the mode for node env. I've found that vite-node is using ssr mode. But I wonder using ssr bundle and ssr.noExternal will result in a better solution than electron-builder. Surely there's some deps that seems not working after bundling. I'll investigate into them later.

Anyway, turn back to web mode like this PR is the wrong way. I'll close this PR now. Thanks for your time.

@Tanimodori Tanimodori closed this Feb 9, 2023
@Tanimodori
Copy link
Contributor Author

I've tried using ssr.external and ssr.noExternal but they are nowhere convenient than build.rollupOptions.external.

  • ssr.external accepts string[]
  • ssr.noExternal accepts string | RegExp | (string | RegExp)[] | true
  • build.rollupOptions.external accepts string | RegExp | (string | RegExp)[] | ((source: string, importer: string | undefined, isResolved: boolean) => boolean | NullValue)

It's odd that you can specify one external pattern and one anti-pattern since you'll wonder how they overwrite the other one once colliding. ssr.external does NOT support RegExp and contains sub-paths (like fs/promises). And neither of them accepts function.

If I specify all node/electron externals in ssr.external and set ssr.noExternal to true, I cannot handle sub-paths. If I specify ssr.noExternal I need to resolve the dependency tree to find out every deps I installed including there sub-deps. I found it extremely inconvenient but the good old build.rollupOptions.external still works:

import nodeBuiltins from 'builtin-modules/static';
import electronBuiltins from 'electron-builtins';
export const config = {
  // ...
  build: {
    ssr: true,
    rollupOptions: {
      // ...
      external: (src) => {
        const [name] = src.split('/');
        const externalNames = [
          ...nodeBuiltins,
          ...nodeBuiltins.map((name) => `node:${name}`),
          ...electronBuiltins,
        ];
        return externalNames.includes(name);
      },
    },
  },
  ssr: { noExternal: true },
  // ...
}

There are a few amendments to ssr.external like vitejs/vite#11817, vitejs/vite#10939, vitejs/vite#11093. But none of they accepts functions. Just use rollup external interfaces, please.

@Tanimodori
Copy link
Contributor Author

The remaining questions are now:

I'll setup some reproductions and see if those problem exist.

@cawa-93
Copy link
Owner

cawa-93 commented Feb 9, 2023

If I specify all node/electron externals in ssr.external and set ssr.noExternal to true,

This have no sence, since noExternal: true have highest priority
https://github.com/vitejs/vite/blob/d953536aae448e2bea0f3a7cb3c0062b16d45597/packages/vite/src/node/ssr/ssrExternal.ts#L42-L44

@Tanimodori
Copy link
Contributor Author

Tanimodori commented Feb 9, 2023

Update: axios (repro), file-type (repro), semver (repro) are fine.

For node-opcua (repro) I've got a different error:

VM543 renderer_init:2 Error: error:06000066:public key routines:OPENSSL_internal:DECODE_ERROR
    at createPrivateKey (VM476 keys:620:12)
    at message_builder.js:37:1
    at Module.<anonymous> (message_builder.js:508:1)
    at Module.<anonymous> (index.ts:6:1)
    at Module._compile (VM508 loader:1174:14)
    at Module._extensions..js (VM508 loader:1229:10)
    at Module.load (VM508 loader:1044:32)
    at Module._load (VM508 loader:885:12)
    at f._load (VM540 asar_bundle:2:13330)
    at o._load (VM543 renderer_init:2:3109)

electron_2HbPP7RmVm

Which tends to be their invalid key was not compatible with node:crypto.

(link to code)

export const invalidPrivateKey = createPrivateKey(`-----BEGIN RSA PRIVATE KEY-----
MB0CAQACAQ8CAwEAAQIBAQIBBQIBAwIBAQIBAQIBAg==
-----END RSA PRIVATE KEY-----`);

@Tanimodori
Copy link
Contributor Author

The new PR is going to be sent from branch fix-ssr

Is there any other issues related to this PR? Let me know if any still exists, thanks!

@cawa-93
Copy link
Owner

cawa-93 commented Feb 9, 2023

@Tanimodori
Copy link
Contributor Author

Tanimodori commented Feb 10, 2023

Update:

@Tanimodori
Copy link
Contributor Author

I was able to perform a fix for the binary node issues.

  • Using build.rollupOptions.output.preserveModules to keep the file structure so those modules who relies on __dirname or __filename will works as intended.
  • Using build.rollupOptions.output.preserveModulesRoot to eliminate packages/preload/src under preserveModules mode so the entry point for main/preload will not be changed.
  • Set build.commonjsOptions.ignoreDynamicRequires to allowing dynamic requires to binary node native modules instead of shabby commonjsRequire which just throw an error.
  • Using vite-plugin-static-copy to manually copy those binary node native modules to the exact place as they were in the original node_modules package.
  • Now rollup will generate node_modules folder under dist. Just set includeSubNodeModules to true to tell electron-builder to copy those modules while compling and we're ready to go.

I've test it in both watch mode and prod builds. Both work flawlessly. I'll apply the fix to previous repros to see if there is any further bugs.

@Tanimodori
Copy link
Contributor Author

I've encountered some build bugs when applying the same methods for main package with rollup and vite. That issue was tracking at vitejs/vite#12012

@Tanimodori
Copy link
Contributor Author

Workaround: set rollupOptions.preserveEntrySignatures to strict

@Tanimodori
Copy link
Contributor Author

Re-checking all repros.

All repros passed except node-opcua which contains buggy code.

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.

None yet

2 participants