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): better logic #3583

Merged
merged 14 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,17 @@
"node/no-unpublished-require": "off",
"node/shebang": "off"
}
},
{
"files": ["packages/plugin/vite/src/**/*.ts"],
"rules": {
"node/no-unpublished-import": [
"error",
{
"allowModules": ["vite"]
}
]
}
}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,6 @@ declare global {
const MAIN_WINDOW_VITE_DEV_SERVER_URL: string;
const MAIN_WINDOW_VITE_NAME: string;

namespace NodeJS {
interface Process {
// Used for hot reload after preload scripts.
viteDevServers: Record<string, import('vite').ViteDevServer>;
}
}

type VitePluginConfig = ConstructorParameters<typeof import('@electron-forge/plugin-vite').VitePlugin>[0];

interface VitePluginRuntimeKeys {
Expand Down
10 changes: 7 additions & 3 deletions packages/plugin/vite/src/Config.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,26 @@
// eslint-disable-next-line node/no-unpublished-import
import type { LibraryOptions } from 'vite';

export interface VitePluginBuildConfig {
/**
* Alias of `build.lib.entry` in `config`.
*/
entry?: LibraryOptions['entry'];
entry: LibraryOptions['entry'];
/**
* Vite config file path.
*/
config: string;
/**
* The build target is main process or preload script.
* @defaultValue 'main'
*/
target?: 'main' | 'preload';
}

export interface VitePluginRendererConfig {
/**
* Human friendly name of your entry point.
*/
name?: string;
name: string;
/**
* Vite config file path.
*/
Expand Down
43 changes: 29 additions & 14 deletions packages/plugin/vite/src/ViteConfig.ts
Original file line number Diff line number Diff line change
@@ -1,33 +1,47 @@
import debug from 'debug';
// eslint-disable-next-line node/no-unpublished-import
import { loadConfigFromFile } from 'vite';
import { loadConfigFromFile, mergeConfig } from 'vite';

import { getConfig as getMainViteConfig } from './config/vite.main.config';
import { getConfig as getPreloadViteConfig } from './config/vite.preload.config';
import { getConfig as getRendererViteConfig } from './config/vite.renderer.config';

import type { VitePluginBuildConfig, VitePluginConfig, VitePluginRendererConfig } from './Config';
// eslint-disable-next-line node/no-unpublished-import
import type { ConfigEnv, UserConfig } from 'vite';

const d = debug('@electron-forge/plugin-vite:ViteConfig');

type Target = NonNullable<VitePluginBuildConfig['target']> | 'renderer';

export default class ViteConfigGenerator {
constructor(private readonly pluginConfig: VitePluginConfig, private readonly projectDir: string, private readonly isProd: boolean) {
d('Config mode:', this.mode);
}

resolveConfig(buildConfig: VitePluginBuildConfig | VitePluginRendererConfig, configEnv: Partial<ConfigEnv> = {}) {
// @see - https://vitejs.dev/config/#conditional-config
configEnv.command ??= this.isProd ? 'build' : 'serve';
// `mode` affects `.env.[mode]` file load.
configEnv.mode ??= this.mode;
async resolveConfig(buildConfig: VitePluginBuildConfig | VitePluginRendererConfig, target: Target): Promise<UserConfig> {
const configEnv: ConfigEnv = {
// @see - https://vitejs.dev/config/#conditional-config
command: this.isProd ? 'build' : 'serve',
// `mode` affects `.env.[mode]` file load.
mode: this.mode,

// Hack! Pass the forge runtime config to the vite config file in the template.
Object.assign(configEnv, {
// Forge extension variables.
root: this.projectDir,
forgeConfig: this.pluginConfig,
forgeConfigSelf: buildConfig,
});
};

// `configEnv` is to be passed as an arguments when the user export a function in `vite.config.js`.
return loadConfigFromFile(configEnv as ConfigEnv, buildConfig.config);
const userConfig = (await loadConfigFromFile(configEnv, buildConfig.config))?.config ?? {};
switch (target) {
case 'main':
return mergeConfig(getMainViteConfig(configEnv as ConfigEnv<'build'>), userConfig);
case 'preload':
return mergeConfig(getPreloadViteConfig(configEnv as ConfigEnv<'build'>), userConfig);
case 'renderer':
return mergeConfig(getRendererViteConfig(configEnv as ConfigEnv<'renderer'>), userConfig);
default:
throw new Error(`Unknown target: ${target}, expected 'main', 'preload' or 'renderer'`);
}
Comment on lines +34 to +44
Copy link

@BurningEnlightenment BurningEnlightenment Sep 18, 2024

Choose a reason for hiding this comment

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

Can we get a bypass? I started with 7.4.0 and have modified the vite.base.config.mjs. Mainly to provide a root if the config gets loaded by vitest and for vite-tsconfig-paths. Without a bypass this would be a breaking change.

EDIT: Has it been considered to provide the vite extensions and default configurations as a library? That would allow me to pluck things like root from the default config and I can call mergeConfig afterwards myself.

Copy link
Member Author

Choose a reason for hiding this comment

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

vite.base.config.mjs in your project will have higher priority than @electron-forge/plugin-vite, and your configuration will still be valid.

Choose a reason for hiding this comment

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

That is only true for things which I explicitly configure, e.g. if I use a different way to implement hot-restart, I'd have to override the plugin applied by default to do nothing… which also looks rather awkward. Therefore it would be nice, if I could just import the pieces I want from this plugin and apply them by myself.

}

get mode(): string {
Expand All @@ -45,7 +59,7 @@ export default class ViteConfigGenerator {
const configs = this.pluginConfig.build
// Prevent load the default `vite.config.js` file.
.filter(({ config }) => config)
.map<Promise<UserConfig>>(async (buildConfig) => (await this.resolveConfig(buildConfig))?.config ?? {});
.map((buildConfig) => this.resolveConfig(buildConfig, buildConfig.target ?? 'main'));

return await Promise.all(configs);
}
Expand All @@ -56,8 +70,9 @@ export default class ViteConfigGenerator {
}

const configs = this.pluginConfig.renderer
// Prevent load the default `vite.config.js` file.
.filter(({ config }) => config)
.map<Promise<UserConfig>>(async (buildConfig) => (await this.resolveConfig(buildConfig))?.config ?? {});
.map((buildConfig) => this.resolveConfig(buildConfig, 'renderer'));

return await Promise.all(configs);
}
Expand Down
16 changes: 4 additions & 12 deletions packages/plugin/vite/src/VitePlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@ import chalk from 'chalk';
import debug from 'debug';
import fs from 'fs-extra';
import { PRESET_TIMER } from 'listr2';
// eslint-disable-next-line node/no-unpublished-import
import { default as vite } from 'vite';

import { getFlatDependencies } from './util/package';
import { onBuildDone } from './util/plugins';
import ViteConfigGenerator from './ViteConfig';

Expand Down Expand Up @@ -95,16 +93,17 @@ Your packaged app may be larger than expected if you dont ignore everything othe
forgeConfig.packagerConfig.ignore = (file: string) => {
if (!file) return false;

// Always starts with `/`
// `file` always starts with `/`
// @see - https://github.com/electron/packager/blob/v18.1.3/src/copy-filter.ts#L89-L93

// Collect the files built by Vite
return !file.startsWith('/.vite');
};
return forgeConfig;
};

packageAfterCopy = async (_forgeConfig: ResolvedForgeConfig, buildPath: string): Promise<void> => {
const pj = await fs.readJson(path.resolve(this.projectDir, 'package.json'));
const flatDependencies = await getFlatDependencies(this.projectDir);

if (!pj.main?.includes('.vite/')) {
throw new Error(`Electron Forge is configured to use the Vite plugin. The plugin expects the
Expand All @@ -116,14 +115,7 @@ the generated files). Instead, it is ${JSON.stringify(pj.main)}`);
delete pj.config.forge;
}

await fs.writeJson(path.resolve(buildPath, 'package.json'), pj, {
spaces: 2,
});

// Copy the dependencies in package.json
for (const dep of flatDependencies) {
await fs.copy(dep.src, path.resolve(buildPath, dep.dest));
}
Comment on lines -119 to -126

Choose a reason for hiding this comment

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

Am I reading this correctly and this removes the functionality to copy dependencies listed in package.json? Is this supposed to land in a forge v8 package or will this breaking change land in a minor version?

Copy link
Member Author

@caoxiemeihao caoxiemeihao Sep 18, 2024

Choose a reason for hiding this comment

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

This should be released as a minor version, because Vite does not build some Node.js modules reliably.
However, most people still want Vite to handle the Node.js modules in dependencies, which can effectively reduce the size of the App build.
In any case, the current change is the choice of most people. If other problems are encountered during the upgrade process, we may need some community-provided Vite plugins to handle them instead of forge.

Choose a reason for hiding this comment

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

In any case, the current change is the choice of most people.

And how has this been measured?

If other problems are encountered during the upgrade process, we may need some community-provided Vite plugins to handle them instead of forge.

Do you mean a vite plugin to include the native binaries in the bundle?

In any case why is it necessary to break existing configuration now? AFAICT it would be possible to add an option to toggle this behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

In general, people who have used @electron-forge/plugin-webpack plugin hope that vite-plugin can have the same design as webpack-plugin, so that they can easily migrate from Webpack to Vite. But in fact, Vite and Webpack are not compatible in many places, and we can only work hard to make it more like Webpack. At least the current PR behavior is consistent with Webpack.
We may need to remove some hack behaviors from Forge, and they will be done by Vite plugin. For example, the copy behavior of dependencies looks very stupid in the eyes of Webpack users, but Vite often cannot solve such problems, especially C/C++ native packages.
Of course, I personally think that the current version of Vite plugin is the result of weighing many aspects, but it is not easy to be accepted by Webpack plugin users.

Copy link
Member Author

Choose a reason for hiding this comment

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

For users, the upgrade will bring some additional problems, which is not only a problem faced by Forge users, but more accurately, it should be a problem that Vite needs to face. I will try my best to provide some additional plugins to help users solve these problems, and also to provide Vite ecosystem with more Webpack-like capabilities.

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case why is it necessary to break existing configuration now? AFAICT it would be possible to add an option to toggle this behavior.

Adding an option to let the user decide whether to copy dependencies would be necessary, as this PR is clearly a departure from previous behavior.
Please let me know if anyone agrees to add this option?

await fs.writeJson(path.resolve(buildPath, 'package.json'), pj, { spaces: 2 });
};

startLogic = async (): Promise<StartResult> => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import { builtinModules } from 'node:module';

import type { AddressInfo } from 'node:net';
import type { ConfigEnv, Plugin, UserConfig } from 'vite';
import pkg from './package.json';
import type { ConfigEnv, Plugin, UserConfig, ViteDevServer } from 'vite';

export const builtins = ['electron', ...builtinModules.map((m) => [m, `node:${m}`]).flat()];

export const external = [...builtins, ...Object.keys('dependencies' in pkg ? (pkg.dependencies as Record<string, unknown>) : {})];
export const external = [...builtins];

// Used for hot reload after preload scripts.
const viteDevServers: Record<string, ViteDevServer> = {};
const viteDevServerUrls: Record<string, string> = {};

export function getBuildConfig(env: ConfigEnv<'build'>): UserConfig {
const { root, mode, command } = env;
Expand Down Expand Up @@ -46,7 +50,7 @@ export function getBuildDefine(env: ConfigEnv<'build'>) {
const define = Object.entries(defineKeys).reduce((acc, [name, keys]) => {
const { VITE_DEV_SERVER_URL, VITE_NAME } = keys;
const def = {
[VITE_DEV_SERVER_URL]: command === 'serve' ? JSON.stringify(process.env[VITE_DEV_SERVER_URL]) : undefined,
[VITE_DEV_SERVER_URL]: command === 'serve' ? JSON.stringify(viteDevServerUrls[VITE_DEV_SERVER_URL]) : undefined,
[VITE_NAME]: JSON.stringify(name),
};
return { ...acc, ...def };
Expand All @@ -61,14 +65,13 @@ export function pluginExposeRenderer(name: string): Plugin {
return {
name: '@electron-forge/plugin-vite:expose-renderer',
configureServer(server) {
process.viteDevServers ??= {};
// Expose server for preload scripts hot reload.
process.viteDevServers[name] = server;
viteDevServers[name] = server;

server.httpServer?.once('listening', () => {
const addressInfo = server.httpServer!.address() as AddressInfo;
const addressInfo = server.httpServer?.address() as AddressInfo;
// Expose env constant for main process use.
process.env[VITE_DEV_SERVER_URL] = `http://localhost:${addressInfo?.port}`;
viteDevServerUrls[VITE_DEV_SERVER_URL] = `http://localhost:${addressInfo?.port}`;
});
},
};
Expand All @@ -79,14 +82,15 @@ export function pluginHotRestart(command: 'reload' | 'restart'): Plugin {
name: '@electron-forge/plugin-vite:hot-restart',
closeBundle() {
if (command === 'reload') {
for (const server of Object.values(process.viteDevServers)) {
for (const server of Object.values(viteDevServers)) {
// Preload scripts hot reload.
server.ws.send({ type: 'full-reload' });
}
} else {
} else if (command === 'restart') {
// Main process hot restart.
// https://github.com/electron/forge/blob/v7.2.0/packages/api/core/src/api/start.ts#L216-L223
process.stdin.emit('data', 'rs');
// TODO: blocked in #3380
BlackHole1 marked this conversation as resolved.
Show resolved Hide resolved
// process.stdin.emit('data', 'rs');
}
},
};
Expand Down
29 changes: 29 additions & 0 deletions packages/plugin/vite/src/config/vite.main.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { type ConfigEnv, mergeConfig, type UserConfig } from 'vite';

import { external, getBuildConfig, getBuildDefine, pluginHotRestart } from './vite.base.config';

export function getConfig(forgeEnv: ConfigEnv<'build'>): UserConfig {
const { forgeConfigSelf } = forgeEnv;
const define = getBuildDefine(forgeEnv);
const config: UserConfig = {
build: {
lib: {
entry: forgeConfigSelf.entry,
fileName: () => '[name].js',
formats: ['cjs'],
},
rollupOptions: {
external,
},
},
plugins: [pluginHotRestart('restart')],
define,
resolve: {
// Load the Node.js entry.
conditions: ['node'],
mainFields: ['module', 'jsnext:main', 'jsnext'],
},
};

return mergeConfig(getBuildConfig(forgeEnv), config);
}
27 changes: 27 additions & 0 deletions packages/plugin/vite/src/config/vite.preload.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { type ConfigEnv, mergeConfig, type UserConfig } from 'vite';

import { external, getBuildConfig, pluginHotRestart } from './vite.base.config';

export function getConfig(forgeEnv: ConfigEnv<'build'>): UserConfig {
const { forgeConfigSelf } = forgeEnv;
const config: UserConfig = {
build: {
rollupOptions: {
external,
// Preload scripts may contain Web assets, so use the `build.rollupOptions.input` instead `build.lib.entry`.
input: forgeConfigSelf.entry,
output: {
format: 'cjs',
// It should not be split chunks.
inlineDynamicImports: true,
entryFileNames: '[name].js',
chunkFileNames: '[name].js',
assetFileNames: '[name].[ext]',
},
},
},
plugins: [pluginHotRestart('reload')],
};

return mergeConfig(getBuildConfig(forgeEnv), config);
}
23 changes: 23 additions & 0 deletions packages/plugin/vite/src/config/vite.renderer.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { type ConfigEnv, type UserConfig } from 'vite';

import { pluginExposeRenderer } from './vite.base.config';

// https://vitejs.dev/config
export function getConfig(forgeEnv: ConfigEnv<'renderer'>) {
const { root, mode, forgeConfigSelf } = forgeEnv;
const name = forgeConfigSelf.name ?? '';

return {
root,
mode,
base: './',
build: {
outDir: `.vite/renderer/${name}`,
},
plugins: [pluginExposeRenderer(name)],
resolve: {
preserveSymlinks: true,
},
clearScreen: false,
} as UserConfig;
}
Loading