From 742cce8b68c4c1900823a66cc90edc1f6691b666 Mon Sep 17 00:00:00 2001 From: James Garbutt <43081j@users.noreply.github.com> Date: Fri, 7 Oct 2022 13:28:30 +0100 Subject: [PATCH 1/5] fix (dev-server-rollup): support child plugins in resolution algorithm It is possible a rollup plug injects its own child plugins by hooking into the rollup `options` hook and mutating the options to have extra plugs. This change introduces a check for such plugins and calls them before the host plugin in case they successfully resolve a path first. Similarly, the `resolve` method of plugins receives an options object to hold state for the resolution plugins. We currently lose this object when calling through to `resolveImport`, which can confuse third-party plugins/resolvers heavily. To fix this, `resolveImport` now also accepts a `resolverOptions` so we can pass it on when calling child plugins. --- .../dev-server-core/src/plugins/Plugin.ts | 1 + .../src/createRollupPluginContextAdapter.ts | 11 ++-- .../dev-server-rollup/src/rollupAdapter.ts | 51 +++++++++++++++---- 3 files changed, 47 insertions(+), 16 deletions(-) diff --git a/packages/dev-server-core/src/plugins/Plugin.ts b/packages/dev-server-core/src/plugins/Plugin.ts index d4616e213..5fe040b3c 100644 --- a/packages/dev-server-core/src/plugins/Plugin.ts +++ b/packages/dev-server-core/src/plugins/Plugin.ts @@ -40,6 +40,7 @@ export interface Plugin { code?: string; column?: number; line?: number; + resolveOptions?: Record; }): ResolveResult | Promise; transformImport?(args: { source: string; diff --git a/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts b/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts index f0936148e..2d9b71ffb 100644 --- a/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts +++ b/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts @@ -74,11 +74,12 @@ export function createRollupPluginContextAdapter< if (!context) throw new Error('Context is required.'); for (const pl of config.plugins ?? []) { - if ( - pl.resolveImport && - (!options.skipSelf || pl.resolveImport !== wdsPlugin.resolveImport) - ) { - const result = await pl.resolveImport({ source, context }); + if (pl.resolveImport && (!options.skipSelf || pl !== wdsPlugin)) { + const result = await pl.resolveImport({ + source, + context, + resolveOptions: options, + }); let resolvedId: string | undefined; if (typeof result === 'string') { resolvedId = result; diff --git a/packages/dev-server-rollup/src/rollupAdapter.ts b/packages/dev-server-rollup/src/rollupAdapter.ts index fcac5f77e..81ff3eb43 100644 --- a/packages/dev-server-rollup/src/rollupAdapter.ts +++ b/packages/dev-server-rollup/src/rollupAdapter.ts @@ -17,7 +17,12 @@ import { setTextContent, } from '@web/dev-server-core/dist/dom5'; import { parse as parseHtml, serialize as serializeHtml } from 'parse5'; -import { CustomPluginOptions, Plugin as RollupPlugin, TransformPluginContext } from 'rollup'; +import { + CustomPluginOptions, + Plugin as RollupPlugin, + TransformPluginContext, + ResolveIdHook, +} from 'rollup'; import { InputOptions } from 'rollup'; import { red, cyan } from 'nanocolors'; @@ -70,6 +75,7 @@ export function rollupAdapter( let fileWatcher: FSWatcher; let config: DevServerCoreConfig; let rootDir: string; + let idResolvers: ResolveIdHook[] = []; function savePluginMeta( id: string, @@ -89,16 +95,33 @@ export function rollupAdapter( ({ rootDir } = config); rollupPluginContexts = await createRollupPluginContexts(rollupInputOptions); + idResolvers = []; + // call the options and buildStart hooks - rollupPlugin.options?.call(rollupPluginContexts.minimalPluginContext, rollupInputOptions) ?? - rollupInputOptions; + const transformedOptions = + (await rollupPlugin.options?.call( + rollupPluginContexts.minimalPluginContext, + rollupInputOptions, + )) ?? rollupInputOptions; rollupPlugin.buildStart?.call( rollupPluginContexts.pluginContext, rollupPluginContexts.normalizedInputOptions, ); + + if (transformedOptions && transformedOptions.plugins) { + for (const subPlugin of transformedOptions.plugins) { + if (subPlugin && subPlugin.resolveId) { + idResolvers.push(subPlugin.resolveId); + } + } + } + + if (rollupPlugin.resolveId) { + idResolvers.push(rollupPlugin.resolveId); + } }, - async resolveImport({ source, context, code, column, line }) { + async resolveImport({ source, context, code, column, line, resolveOptions }) { if (context.response.is('html') && source.startsWith('�')) { // when serving HTML a null byte gets parsed as an unknown character // we remap it to a null byte here so that it is handled properly downstream @@ -109,7 +132,7 @@ export function rollupAdapter( // if we just transformed this file and the import is an absolute file path // we need to rewrite it to a browser path const injectedFilePath = path.normalize(source).startsWith(rootDir); - if (!injectedFilePath && !rollupPlugin.resolveId) { + if (!injectedFilePath && idResolvers.length === 0) { return; } @@ -146,12 +169,18 @@ export function rollupAdapter( } } - let result = await rollupPlugin.resolveId?.call( - rollupPluginContext, - resolvableImport, - filePath, - { isEntry: false }, - ); + let result = null; + + for (const idResolver of idResolvers) { + result = await idResolver.call(rollupPluginContext, resolvableImport, filePath, { + ...resolveOptions, + isEntry: false, + }); + + if (result) { + break; + } + } if (!result && injectedFilePath) { // the import is a file path but it was not resolved by this plugin From 0248872345fdd7c11319744e4e5fb79acc944515 Mon Sep 17 00:00:00 2001 From: James Garbutt <43081j@users.noreply.github.com> Date: Thu, 20 Oct 2022 19:48:36 +0100 Subject: [PATCH 2/5] fix (dev-server-rollup): respect skipSelf --- .../dev-server-core/src/plugins/Plugin.ts | 6 +++- .../dev-server-rollup/src/rollupAdapter.ts | 17 ++++++++++ .../test/node/plugins/commonjs.test.ts | 31 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/packages/dev-server-core/src/plugins/Plugin.ts b/packages/dev-server-core/src/plugins/Plugin.ts index 5fe040b3c..d3a023275 100644 --- a/packages/dev-server-core/src/plugins/Plugin.ts +++ b/packages/dev-server-core/src/plugins/Plugin.ts @@ -40,7 +40,11 @@ export interface Plugin { code?: string; column?: number; line?: number; - resolveOptions?: Record; + resolveOptions?: { + custom?: Record; + isEntry?: boolean; + skipSelf?: boolean; + }; }): ResolveResult | Promise; transformImport?(args: { source: string; diff --git a/packages/dev-server-rollup/src/rollupAdapter.ts b/packages/dev-server-rollup/src/rollupAdapter.ts index 81ff3eb43..4e8c20dcb 100644 --- a/packages/dev-server-rollup/src/rollupAdapter.ts +++ b/packages/dev-server-rollup/src/rollupAdapter.ts @@ -60,6 +60,8 @@ export interface RollupAdapterOptions { throwOnUnresolvedImport?: boolean; } +const resolverCache = new WeakMap>>(); + export function rollupAdapter( rollupPlugin: RollupPlugin, rollupInputOptions: Partial = {}, @@ -171,6 +173,21 @@ export function rollupAdapter( let result = null; + const resolverCacheForContext = + resolverCache.get(context) ?? new WeakMap>(); + resolverCache.set(context, resolverCacheForContext); + const resolverCacheForPlugin = resolverCacheForContext.get(wdsPlugin) ?? new Set(); + resolverCacheForContext.set(wdsPlugin, resolverCacheForPlugin); + + const cacheKey = `${resolvableImport}:${filePath}`; + + if (resolveOptions?.skipSelf) { + if (resolverCacheForPlugin.has(cacheKey)) { + return undefined; + } + resolverCacheForPlugin.add(cacheKey); + } + for (const idResolver of idResolvers) { result = await idResolver.call(rollupPluginContext, resolvableImport, filePath, { ...resolveOptions, diff --git a/packages/dev-server-rollup/test/node/plugins/commonjs.test.ts b/packages/dev-server-rollup/test/node/plugins/commonjs.test.ts index a5c3c138f..e13aed548 100644 --- a/packages/dev-server-rollup/test/node/plugins/commonjs.test.ts +++ b/packages/dev-server-rollup/test/node/plugins/commonjs.test.ts @@ -3,8 +3,10 @@ import { runTests } from '@web/test-runner-core/test-helpers'; import { resolve } from 'path'; import { chromeLauncher } from '@web/test-runner-chrome'; +import * as path from 'path'; import { createTestServer, fetchText, expectIncludes } from '../test-helpers'; import { fromRollup } from '../../../src/index'; +import { nodeResolvePlugin } from '@web/dev-server'; const commonjs = fromRollup(rollupCommonjs); @@ -135,6 +137,35 @@ exports.default = _default;`; } }); + it('can transform modules which require node-resolved modules', async () => { + const rootDir = path.resolve(__dirname, '..', 'fixtures', 'basic'); + const { server, host } = await createTestServer({ + plugins: [ + { + name: 'test', + serve(context) { + if (context.path === '/foo.js') { + return 'import {expect} from "chai"; export {expect};'; + } + }, + }, + commonjs(), + nodeResolvePlugin(rootDir, false, {}), + ], + }); + + try { + const text = await fetchText(`${host}/foo.js`); + expectIncludes( + text, + 'import {expect} from "/__wds-outside-root__/6/node_modules/chai/index.mjs"', + ); + expectIncludes(text, 'export {expect};'); + } finally { + server.stop(); + } + }); + it('can transform modules which require other modules', async () => { const { server, host } = await createTestServer({ plugins: [ From 3308f7966cae84582dc7d1daf17eca3c53198b4a Mon Sep 17 00:00:00 2001 From: Luca Haneklau Date: Wed, 18 Jan 2023 18:59:04 +0100 Subject: [PATCH 3/5] fix (dev-server-rollup): fix ImportSkip map order --- .../dev-server-core/src/plugins/Plugin.ts | 1 + .../src/createRollupPluginContextAdapter.ts | 8 +++++--- .../dev-server-rollup/src/rollupAdapter.ts | 19 ++++++++++++------- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/dev-server-core/src/plugins/Plugin.ts b/packages/dev-server-core/src/plugins/Plugin.ts index d3a023275..a65fd5897 100644 --- a/packages/dev-server-core/src/plugins/Plugin.ts +++ b/packages/dev-server-core/src/plugins/Plugin.ts @@ -46,6 +46,7 @@ export interface Plugin { skipSelf?: boolean; }; }): ResolveResult | Promise; + resolveImportSkip?(context: Context, source: string, importer: string): void; transformImport?(args: { source: string; context: Context; diff --git a/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts b/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts index 2d9b71ffb..f605f4893 100644 --- a/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts +++ b/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts @@ -70,15 +70,17 @@ export function createRollupPluginContextAdapter< throw new Error('Emitting files is not yet supported'); }, - async resolve(source: string, importer: string, options: { skipSelf: boolean }) { + async resolve(source: string, importer: string, options: { isEntry: boolean, skipSelf: boolean, custom: Record }) { if (!context) throw new Error('Context is required.'); + if (options.skipSelf) wdsPlugin.resolveImportSkip?.(context, source, importer); + for (const pl of config.plugins ?? []) { if (pl.resolveImport && (!options.skipSelf || pl !== wdsPlugin)) { const result = await pl.resolveImport({ source, context, - resolveOptions: options, + resolveOptions: {isEntry: options.isEntry, custom: options.custom}, }); let resolvedId: string | undefined; if (typeof result === 'string') { @@ -98,7 +100,7 @@ export function createRollupPluginContextAdapter< } }, - async resolveId(source: string, importer: string, options: { skipSelf: boolean }) { + async resolveId(source: string, importer: string, options: { isEntry: boolean, skipSelf: boolean, custom: Record }) { const resolveResult = await this.resolve(source, importer, options); if (typeof resolveResult === 'string') { return resolveResult; diff --git a/packages/dev-server-rollup/src/rollupAdapter.ts b/packages/dev-server-rollup/src/rollupAdapter.ts index 4e8c20dcb..e385f65d9 100644 --- a/packages/dev-server-rollup/src/rollupAdapter.ts +++ b/packages/dev-server-rollup/src/rollupAdapter.ts @@ -123,6 +123,16 @@ export function rollupAdapter( } }, + resolveImportSkip(context: Context, source: string, importer: string) { + const resolverCacheForContext = + resolverCache.get(context) ?? new WeakMap>(); + resolverCache.set(context, resolverCacheForContext); + const resolverCacheForPlugin = resolverCacheForContext.get(wdsPlugin) ?? new Set(); + resolverCacheForContext.set(wdsPlugin, resolverCacheForPlugin); + + resolverCacheForPlugin.add(`${source}:${importer}`); + }, + async resolveImport({ source, context, code, column, line, resolveOptions }) { if (context.response.is('html') && source.startsWith('�')) { // when serving HTML a null byte gets parsed as an unknown character @@ -179,13 +189,8 @@ export function rollupAdapter( const resolverCacheForPlugin = resolverCacheForContext.get(wdsPlugin) ?? new Set(); resolverCacheForContext.set(wdsPlugin, resolverCacheForPlugin); - const cacheKey = `${resolvableImport}:${filePath}`; - - if (resolveOptions?.skipSelf) { - if (resolverCacheForPlugin.has(cacheKey)) { - return undefined; - } - resolverCacheForPlugin.add(cacheKey); + if (resolverCacheForPlugin.has(`${source}:${filePath}`)) { + return undefined; } for (const idResolver of idResolvers) { From e6ea158bb06223f53ee1e194032a234d1c8c5ff3 Mon Sep 17 00:00:00 2001 From: James Garbutt <43081j@users.noreply.github.com> Date: Wed, 18 Jan 2023 18:25:02 +0000 Subject: [PATCH 4/5] feat (dev-server-rollup): pass through resolve options --- .../src/createRollupPluginContextAdapter.ts | 21 ++++++++++++++----- .../dev-server-rollup/src/rollupAdapter.ts | 2 +- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts b/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts index f605f4893..6f72c8d19 100644 --- a/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts +++ b/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts @@ -8,6 +8,11 @@ import { ModuleInfo, } from 'rollup'; +interface ResolveOptions { + skipSelf?: boolean; + [key: string]: unknown; +} + export function createRollupPluginContextAdapter< T extends PluginContext | MinimalPluginContext | TransformPluginContext, >( @@ -70,17 +75,19 @@ export function createRollupPluginContextAdapter< throw new Error('Emitting files is not yet supported'); }, - async resolve(source: string, importer: string, options: { isEntry: boolean, skipSelf: boolean, custom: Record }) { + async resolve(source: string, importer: string, options: ResolveOptions) { if (!context) throw new Error('Context is required.'); - if (options.skipSelf) wdsPlugin.resolveImportSkip?.(context, source, importer); + const { skipSelf, ...resolveOptions } = options; + + if (skipSelf) wdsPlugin.resolveImportSkip?.(context, source, importer); for (const pl of config.plugins ?? []) { - if (pl.resolveImport && (!options.skipSelf || pl !== wdsPlugin)) { + if (pl.resolveImport && (!skipSelf || pl !== wdsPlugin)) { const result = await pl.resolveImport({ source, context, - resolveOptions: {isEntry: options.isEntry, custom: options.custom}, + resolveOptions, }); let resolvedId: string | undefined; if (typeof result === 'string') { @@ -100,7 +107,11 @@ export function createRollupPluginContextAdapter< } }, - async resolveId(source: string, importer: string, options: { isEntry: boolean, skipSelf: boolean, custom: Record }) { + async resolveId( + source: string, + importer: string, + options: { isEntry: boolean; skipSelf: boolean; custom: Record }, + ) { const resolveResult = await this.resolve(source, importer, options); if (typeof resolveResult === 'string') { return resolveResult; diff --git a/packages/dev-server-rollup/src/rollupAdapter.ts b/packages/dev-server-rollup/src/rollupAdapter.ts index e385f65d9..74403b6ec 100644 --- a/packages/dev-server-rollup/src/rollupAdapter.ts +++ b/packages/dev-server-rollup/src/rollupAdapter.ts @@ -129,7 +129,7 @@ export function rollupAdapter( resolverCache.set(context, resolverCacheForContext); const resolverCacheForPlugin = resolverCacheForContext.get(wdsPlugin) ?? new Set(); resolverCacheForContext.set(wdsPlugin, resolverCacheForPlugin); - + resolverCacheForPlugin.add(`${source}:${importer}`); }, From fd71f22c72831b75e5bfda39e251327d989ebc41 Mon Sep 17 00:00:00 2001 From: James Garbutt <43081j@users.noreply.github.com> Date: Wed, 18 Jan 2023 18:42:49 +0000 Subject: [PATCH 5/5] feat: extract resolve options to plugin definition --- packages/dev-server-core/src/index.ts | 2 +- packages/dev-server-core/src/plugins/Plugin.ts | 12 +++++++----- .../src/createRollupPluginContextAdapter.ts | 13 +++++++------ 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/dev-server-core/src/index.ts b/packages/dev-server-core/src/index.ts index 63c596468..2bcfb0c8d 100644 --- a/packages/dev-server-core/src/index.ts +++ b/packages/dev-server-core/src/index.ts @@ -6,7 +6,7 @@ import WebSocket from 'ws'; export { WebSocket }; export { DevServer } from './server/DevServer'; -export { Plugin, ServerStartParams } from './plugins/Plugin'; +export { Plugin, ServerStartParams, ResolveOptions } from './plugins/Plugin'; export { DevServerCoreConfig, MimeTypeMappings } from './server/DevServerCoreConfig'; export { WebSocketsManager, WebSocketData } from './web-sockets/WebSocketsManager'; export { diff --git a/packages/dev-server-core/src/plugins/Plugin.ts b/packages/dev-server-core/src/plugins/Plugin.ts index a65fd5897..54cb52b76 100644 --- a/packages/dev-server-core/src/plugins/Plugin.ts +++ b/packages/dev-server-core/src/plugins/Plugin.ts @@ -26,6 +26,12 @@ export interface ServerStartParams { webSockets?: WebSocketsManager; } +export interface ResolveOptions { + isEntry?: boolean; + skipSelf?: boolean; + [key: string]: unknown; +} + export interface Plugin { name: string; injectWebSocket?: boolean; @@ -40,11 +46,7 @@ export interface Plugin { code?: string; column?: number; line?: number; - resolveOptions?: { - custom?: Record; - isEntry?: boolean; - skipSelf?: boolean; - }; + resolveOptions?: ResolveOptions; }): ResolveResult | Promise; resolveImportSkip?(context: Context, source: string, importer: string): void; transformImport?(args: { diff --git a/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts b/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts index 6f72c8d19..ff88d90c9 100644 --- a/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts +++ b/packages/dev-server-rollup/src/createRollupPluginContextAdapter.ts @@ -1,5 +1,11 @@ import path from 'path'; -import { DevServerCoreConfig, FSWatcher, Plugin as WdsPlugin, Context } from '@web/dev-server-core'; +import { + DevServerCoreConfig, + FSWatcher, + Plugin as WdsPlugin, + Context, + ResolveOptions, +} from '@web/dev-server-core'; import { PluginContext, MinimalPluginContext, @@ -8,11 +14,6 @@ import { ModuleInfo, } from 'rollup'; -interface ResolveOptions { - skipSelf?: boolean; - [key: string]: unknown; -} - export function createRollupPluginContextAdapter< T extends PluginContext | MinimalPluginContext | TransformPluginContext, >(