-
-
Notifications
You must be signed in to change notification settings - Fork 522
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
Changes from 13 commits
5485969
34c6446
c701336
182fa3f
bb9ee55
1bae6ab
75815ac
da1ba8d
29d4e0b
5880756
a0148b5
9c6ca78
1ae795f
ea21a2a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
|
@@ -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 | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
And how has this been measured?
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, people who have used There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Adding an option to let the user decide whether to copy |
||
await fs.writeJson(path.resolve(buildPath, 'package.json'), pj, { spaces: 2 }); | ||
}; | ||
|
||
startLogic = async (): Promise<StartResult> => { | ||
|
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); | ||
} |
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); | ||
} |
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; | ||
} |
There was a problem hiding this comment.
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 thevite.base.config.mjs
. Mainly to provide aroot
if the config gets loaded byvitest
and forvite-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 callmergeConfig
afterwards myself.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.