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!: worker.plugins is a function #14685

Merged
merged 5 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/config/worker-options.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ Output format for worker bundle.

## worker.plugins

- **Type:** [`(Plugin | Plugin[])[]`](./shared-options#plugins)
- **Type:** [`() => (Plugin | Plugin[])[]`](./shared-options#plugins)

Vite plugins that apply to worker bundle. Note that [config.plugins](./shared-options#plugins) only applies to workers in dev, it should be configured here instead for build.
Vite plugins that apply to the worker bundles. Note that [config.plugins](./shared-options#plugins) only applies to workers in dev, it should be configured here instead for build.
The function should return new plugin instances as they are used in parallel rollup worker builds. As such, modifying `config.worker` options in the `config` hook will be ignored.
patak-dev marked this conversation as resolved.
Show resolved Hide resolved

## worker.rollupOptions

Expand Down
4 changes: 4 additions & 0 deletions docs/guide/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ See the [troubleshooting guide](/guide/troubleshooting.html#vite-cjs-node-api-de

## General Changes

### `worker.plugins` is now a function

In Vite 4, `worker.plugins` accepted an array of plugins (`(Plugin | Plugin[])[]`). From Vite 5, it needs to be configured as a function that returns an array of plugins (`() => (Plugin | Plugin[])[]`). This change is required so parallel worker builds run more consistently and predictably.

### Allow path containing `.` to fallback to index.html

In Vite 4, accessing a path containing `.` did not fallback to index.html even if `appType` is set to `'SPA'` (default).
Expand Down
130 changes: 76 additions & 54 deletions packages/vite/src/node/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,9 +259,11 @@ export interface UserConfig {
*/
format?: 'es' | 'iife'
/**
* Vite plugins that apply to worker bundle
* Vite plugins that apply to worker bundle. The plugins retured by this function
* should be new instances every time it is called, because they are used for each
* rollup worker bundling process.
*/
plugins?: PluginOption[]
plugins?: () => PluginOption[]
/**
* Rollup options to build worker bundle
*/
Expand Down Expand Up @@ -316,9 +318,9 @@ export interface LegacyOptions {
*/
}

export interface ResolvedWorkerOptions extends PluginHookUtils {
export interface ResolvedWorkerOptions {
format: 'es' | 'iife'
plugins: Plugin[]
plugins: () => Promise<Plugin[]>
rollupOptions: RollupOptions
}

Expand Down Expand Up @@ -440,12 +442,6 @@ export async function resolveConfig(
return p.apply === command
}
}
// Some plugins that aren't intended to work in the bundling of workers (doing post-processing at build time for example).
// And Plugins may also have cached that could be corrupted by being used in these extra rollup calls.
// So we need to separate the worker plugin from the plugin that vite needs to run.
const rawWorkerUserPlugins = (
(await asyncFlatten(config.worker?.plugins || [])) as Plugin[]
).filter(filterPlugin)

// resolve plugins
const rawUserPlugins = (
Expand Down Expand Up @@ -659,27 +655,74 @@ export async function resolveConfig(

const BASE_URL = resolvedBase

// resolve worker
let workerConfig = mergeConfig({}, config)
const [workerPrePlugins, workerNormalPlugins, workerPostPlugins] =
sortUserPlugins(rawWorkerUserPlugins)
let resolved: ResolvedConfig

let createUserWorkerPlugins = config.worker?.plugins
if (Array.isArray(createUserWorkerPlugins)) {
// @ts-expect-error backward compatibility
createUserWorkerPlugins = () => config.worker?.plugins

logger.warn(
colors.yellow(
`worker.plugins is now a function that returns an array of plugins. ` +
`Please update your Vite config accordingly.\n`,
),
)
}

const createWorkerPlugins = async function () {
// Some plugins that aren't intended to work in the bundling of workers (doing post-processing at build time for example).
// And Plugins may also have cached that could be corrupted by being used in these extra rollup calls.
// So we need to separate the worker plugin from the plugin that vite needs to run.
const rawWorkerUserPlugins = (
(await asyncFlatten(createUserWorkerPlugins?.() || [])) as Plugin[]
).filter(filterPlugin)

// resolve worker
let workerConfig = mergeConfig({}, config)
const [workerPrePlugins, workerNormalPlugins, workerPostPlugins] =
sortUserPlugins(rawWorkerUserPlugins)

// run config hooks
const workerUserPlugins = [
...workerPrePlugins,
...workerNormalPlugins,
...workerPostPlugins,
]
workerConfig = await runConfigHook(
workerConfig,
workerUserPlugins,
configEnv,
)

const workerResolved: ResolvedConfig = {
...workerConfig,
...resolved,
isWorker: true,
mainConfig: resolved,
}
const resolvedWorkerPlugins = await resolvePlugins(
workerResolved,
workerPrePlugins,
workerNormalPlugins,
workerPostPlugins,
)

// run configResolved hooks
createPluginHookUtils(resolvedWorkerPlugins)
.getSortedPluginHooks('configResolved')
.map((hook) => hook(workerResolved))

return resolvedWorkerPlugins
}

// run config hooks
const workerUserPlugins = [
...workerPrePlugins,
...workerNormalPlugins,
...workerPostPlugins,
]
workerConfig = await runConfigHook(workerConfig, workerUserPlugins, configEnv)
const resolvedWorkerOptions: ResolvedWorkerOptions = {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can move the workers resolve code as a function like resolveWorkerOptions? So it fits the pattern like the other more complex options. Might need a new file beside config.ts.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that but it is using internal functions like filterPlugin, and I wanted to avoid changing unrelated parts on this PR. Maybe we could do that refactoring in another PR?

format: workerConfig.worker?.format || 'iife',
plugins: [],
rollupOptions: workerConfig.worker?.rollupOptions || {},
getSortedPlugins: undefined!,
getSortedPluginHooks: undefined!,
format: config.worker?.format || 'iife',
plugins: createWorkerPlugins,
rollupOptions: config.worker?.rollupOptions || {},
}

const resolvedConfig: ResolvedConfig = {
resolved = {
configFile: configFile ? normalizePath(configFile) : undefined,
configFileDependencies: configFileDependencies.map((name) =>
normalizePath(path.resolve(name)),
Expand Down Expand Up @@ -741,11 +784,10 @@ export async function resolveConfig(
getSortedPlugins: undefined!,
getSortedPluginHooks: undefined!,
}
const resolved: ResolvedConfig = {
resolved = {
...config,
...resolvedConfig,
...resolved,
}

;(resolved.plugins as Plugin[]) = await resolvePlugins(
resolved,
prePlugins,
Expand All @@ -754,32 +796,12 @@ export async function resolveConfig(
)
Object.assign(resolved, createPluginHookUtils(resolved.plugins))

const workerResolved: ResolvedConfig = {
...workerConfig,
...resolvedConfig,
isWorker: true,
mainConfig: resolved,
}
resolvedConfig.worker.plugins = await resolvePlugins(
workerResolved,
workerPrePlugins,
workerNormalPlugins,
workerPostPlugins,
)
Object.assign(
resolvedConfig.worker,
createPluginHookUtils(resolvedConfig.worker.plugins),
)

// call configResolved hooks
await Promise.all([
...resolved
await Promise.all(
resolved
.getSortedPluginHooks('configResolved')
.map((hook) => hook(resolved)),
...resolvedConfig.worker
.getSortedPluginHooks('configResolved')
.map((hook) => hook(workerResolved)),
])
)

// validate config

Expand All @@ -804,7 +826,7 @@ export async function resolveConfig(
plugins: resolved.plugins.map((p) => p.name),
worker: {
...resolved.worker,
plugins: resolved.worker.plugins.map((p) => p.name),
plugins: `() => plugins`,
},
})

Expand Down
27 changes: 2 additions & 25 deletions packages/vite/src/node/plugins/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,30 +41,7 @@ function saveEmitWorkerAsset(
workerMap.assets.set(fileName, asset)
}

// Ensure that only one rollup build is called at the same time to avoid
// leaking state in plugins between worker builds.
// TODO: Review if we can parallelize the bundling of workers.
const workerConfigSemaphore = new WeakMap<
ResolvedConfig,
Promise<OutputChunk>
>()
export async function bundleWorkerEntry(
config: ResolvedConfig,
id: string,
query: Record<string, string> | null,
): Promise<OutputChunk> {
const processing = workerConfigSemaphore.get(config)
if (processing) {
await processing
return bundleWorkerEntry(config, id, query)
}
const promise = serialBundleWorkerEntry(config, id, query)
workerConfigSemaphore.set(config, promise)
promise.then(() => workerConfigSemaphore.delete(config))
return promise
}

async function serialBundleWorkerEntry(
async function bundleWorkerEntry(
config: ResolvedConfig,
id: string,
query: Record<string, string> | null,
Expand All @@ -75,7 +52,7 @@ async function serialBundleWorkerEntry(
const bundle = await rollup({
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's useful if there's a debug log here that shows the plugin names like we had here?
https://github.com/vitejs/vite/pull/14685/files#diff-11e17761d4ecfee8f8fde15c6d79b7bc0260176396a30dfd8e6f6bbaf5af4745R829

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to see how to do this only once because it will be too noisy if not having the logs everytime for projects that have a lot of workers. I'd say we merge it so it is part of the release and do this in a future PR.

...rollupOptions,
input: cleanUrl(id),
plugins,
plugins: await plugins(),
onwarn(warning, warn) {
onRollupWarning(warning, warn, config)
},
Expand Down
16 changes: 16 additions & 0 deletions packages/vite/src/node/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,16 @@ export function removeComments(raw: string): string {
return raw.replace(multilineCommentsRE, '').replace(singlelineCommentsRE, '')
}

function backwardCompatibleWorkerPlugins(plugins: any) {
if (Array.isArray(plugins)) {
return plugins
}
if (typeof plugins === 'function') {
return plugins()
}
return []
}

function mergeConfigRecursively(
defaults: Record<string, any>,
overrides: Record<string, any>,
Expand Down Expand Up @@ -1045,6 +1055,12 @@ function mergeConfigRecursively(
) {
merged[key] = true
continue
} else if (key === 'plugins' && rootPath === 'worker') {
merged[key] = () => [
...backwardCompatibleWorkerPlugins(existing),
...backwardCompatibleWorkerPlugins(value),
]
continue
}

if (Array.isArray(existing) || Array.isArray(value)) {
Expand Down
20 changes: 19 additions & 1 deletion playground/worker/__tests__/es/es-worker.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,30 @@ test('worker emitted and import.meta.url in nested worker (serve)', async () =>
)
})

test('deeply nested workers', async () => {
await untilUpdated(
async () => page.textContent('.deeply-nested-worker'),
/Hello\sfrom\sroot.*\/es\/.+deeply-nested-worker\.js/,
true,
)
await untilUpdated(
async () => page.textContent('.deeply-nested-second-worker'),
/Hello\sfrom\ssecond.*\/es\/.+second-worker\.js/,
true,
)
await untilUpdated(
async () => page.textContent('.deeply-nested-third-worker'),
/Hello\sfrom\sthird.*\/es\/.+third-worker\.js/,
true,
)
})

describe.runIf(isBuild)('build', () => {
// assert correct files
test('inlined code generation', async () => {
const assetsDir = path.resolve(testDir, 'dist/es/assets')
const files = fs.readdirSync(assetsDir)
expect(files.length).toBe(28)
expect(files.length).toBe(32)
const index = files.find((f) => f.includes('main-module'))
const content = fs.readFileSync(path.resolve(assetsDir, index), 'utf-8')
const worker = files.find((f) => f.includes('my-worker'))
Expand Down
4 changes: 2 additions & 2 deletions playground/worker/__tests__/iife/iife-worker.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,10 @@ describe.runIf(isBuild)('build', () => {
test('inlined code generation', async () => {
const assetsDir = path.resolve(testDir, 'dist/iife/assets')
const files = fs.readdirSync(assetsDir)
expect(files.length).toBe(16)
expect(files.length).toBe(20)
const index = files.find((f) => f.includes('main-module'))
const content = fs.readFileSync(path.resolve(assetsDir, index), 'utf-8')
const worker = files.find((f) => f.includes('my-worker'))
const worker = files.find((f) => f.includes('worker_entry-my-worker'))
const workerContent = fs.readFileSync(
path.resolve(assetsDir, worker),
'utf-8',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe.runIf(isBuild)('build', () => {

const files = fs.readdirSync(assetsDir)
// should have 2 worker chunk
expect(files.length).toBe(32)
expect(files.length).toBe(40)
const index = files.find((f) => f.includes('main-module'))
const content = fs.readFileSync(path.resolve(assetsDir, index), 'utf-8')
const indexSourcemap = getSourceMapUrl(content)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe.runIf(isBuild)('build', () => {

const files = fs.readdirSync(assetsDir)
// should have 2 worker chunk
expect(files.length).toBe(16)
expect(files.length).toBe(20)
const index = files.find((f) => f.includes('main-module'))
const content = fs.readFileSync(path.resolve(assetsDir, index), 'utf-8')
const indexSourcemap = getSourceMapUrl(content)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ describe.runIf(isBuild)('build', () => {
const assetsDir = path.resolve(testDir, 'dist/iife-sourcemap/assets')
const files = fs.readdirSync(assetsDir)
// should have 2 worker chunk
expect(files.length).toBe(32)
expect(files.length).toBe(40)
const index = files.find((f) => f.includes('main-module'))
const content = fs.readFileSync(path.resolve(assetsDir, index), 'utf-8')
const indexSourcemap = getSourceMapUrl(content)
Expand Down
19 changes: 19 additions & 0 deletions playground/worker/deeply-nested-second-worker.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
self.postMessage({
type: 'deeplyNestedSecondWorker',
data: [
'Hello from second level nested worker',
import.meta.env.BASE_URL,
self.location.url,
import.meta.url,
].join(' '),
})

const deeplyNestedThirdWorker = new Worker(
new URL('deeply-nested-third-worker.js', import.meta.url),
{ type: 'module' },
)
deeplyNestedThirdWorker.addEventListener('message', (ev) => {
self.postMessage(ev.data)
})

console.log('deeply-nested-second-worker.js')
11 changes: 11 additions & 0 deletions playground/worker/deeply-nested-third-worker.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
self.postMessage({
type: 'deeplyNestedThirdWorker',
data: [
'Hello from third level nested worker',
import.meta.env.BASE_URL,
self.location.url,
import.meta.url,
].join(' '),
})

console.log('deeply-nested-third-worker.js')