From a8235cfe984e2d512433bc5e34fd638ff1605f0a Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Wed, 16 Feb 2022 07:04:15 +0100 Subject: [PATCH 1/4] Add dependencies to shouldTransform hook --- src/Module.ts | 5 +++++ src/ModuleLoader.ts | 2 ++ src/rollup/types.d.ts | 3 +++ test/incremental/index.js | 43 +++++++++++++++++++++++++++++++++------ 4 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/Module.ts b/src/Module.ts index f36ef51d5a3..2810451ef8e 100644 --- a/src/Module.ts +++ b/src/Module.ts @@ -279,6 +279,8 @@ export default class Module { .filter(Boolean) as ResolvedId[]; }, get dynamicallyImportedIds() { + // We cannot use this.dynamicDependencies because this is needed before + // dynamicDependencies are populated const dynamicallyImportedIds: string[] = []; for (const { id } of dynamicImports) { if (id) { @@ -316,6 +318,8 @@ export default class Module { return Array.from(sources, source => module.resolvedIds[source]).filter(Boolean); }, get importedIds() { + // We cannot use this.dependencies because this is needed before + // dependencies are populated return Array.from(sources, source => module.resolvedIds[source]?.id).filter(Boolean); }, get importers() { @@ -781,6 +785,7 @@ export default class Module { code: this.info.code!, customTransformCache: this.customTransformCache, dependencies: Array.from(this.dependencies, getId), + dynamicDependencies: Array.from(this.dynamicDependencies, getId), id: this.id, meta: this.info.meta, moduleSideEffects: this.info.moduleSideEffects, diff --git a/src/ModuleLoader.ts b/src/ModuleLoader.ts index efa402f6a80..3cae7127a71 100644 --- a/src/ModuleLoader.ts +++ b/src/ModuleLoader.ts @@ -280,7 +280,9 @@ export class ModuleLoader { { ast: cachedModule.ast, code: cachedModule.code, + dynamicallyImportedIds: cachedModule.dynamicDependencies, id: cachedModule.id, + importedIds: cachedModule.dependencies, meta: cachedModule.meta, moduleSideEffects: cachedModule.moduleSideEffects, syntheticNamedExports: cachedModule.syntheticNamedExports diff --git a/src/rollup/types.d.ts b/src/rollup/types.d.ts index baf2b6c553a..8081dd07934 100644 --- a/src/rollup/types.d.ts +++ b/src/rollup/types.d.ts @@ -116,6 +116,7 @@ export interface TransformModuleJSON { export interface ModuleJSON extends TransformModuleJSON, ModuleOptions { ast: AcornNode; dependencies: string[]; + dynamicDependencies: string[]; id: string; transformFiles: EmittedFile[] | undefined; } @@ -251,7 +252,9 @@ export type ShouldTransformCachedModuleHook = ( options: { ast: AcornNode; code: string; + dynamicallyImportedIds: string[]; id: string; + importedIds: string[]; meta: CustomPluginOptions; moduleSideEffects: boolean | 'no-treeshake'; syntheticNamedExports: boolean | string; diff --git a/test/incremental/index.js b/test/incremental/index.js index 7de0fb47aeb..52458478ded 100644 --- a/test/incremental/index.js +++ b/test/incremental/index.js @@ -338,10 +338,22 @@ describe('incremental', () => { }); it('runs shouldTransformCachedModule when using a cached module', async () => { + modules = { + entry: `import foo from 'foo'; export default foo; import('bar').then(console.log);`, + foo: `export default 42`, + bar: `export default 21` + }; let shouldTransformCachedModuleCalls = 0; const transformPlugin = { - async shouldTransformCachedModule({ ast, id, meta, ...other }) { + async shouldTransformCachedModule({ + ast, + id, + meta, + importedIds, + dynamicallyImportedIds, + ...other + }) { shouldTransformCachedModuleCalls++; assert.strictEqual(ast.type, 'Program'); assert.deepStrictEqual(other, { @@ -352,10 +364,19 @@ describe('incremental', () => { switch (id) { case 'foo': assert.deepStrictEqual(meta, { transform: { calls: 1, id } }); + assert.deepStrictEqual(importedIds, []); + assert.deepStrictEqual(dynamicallyImportedIds, []); // we return promises to ensure they are awaited return Promise.resolve(false); + case 'bar': + assert.deepStrictEqual(meta, { transform: { calls: 1, id } }); + assert.deepStrictEqual(importedIds, []); + assert.deepStrictEqual(dynamicallyImportedIds, []); + return Promise.resolve(false); case 'entry': assert.deepStrictEqual(meta, { transform: { calls: 0, id } }); + assert.deepStrictEqual(importedIds, ['foo']); + assert.deepStrictEqual(dynamicallyImportedIds, ['bar']); return Promise.resolve(true); default: throw new Error(`Unexpected id ${id}.`); @@ -369,8 +390,12 @@ describe('incremental', () => { input: 'entry', plugins: [transformPlugin, plugin] }); - assert.strictEqual(shouldTransformCachedModuleCalls, 0); - assert.strictEqual(transformCalls, 2); + assert.strictEqual( + shouldTransformCachedModuleCalls, + 0, + 'initial shouldTransformCachedModule calls' + ); + assert.strictEqual(transformCalls, 3, 'initial transform calls'); const { cache: { modules: cachedModules } @@ -379,11 +404,17 @@ describe('incremental', () => { plugins: [transformPlugin, plugin], cache }); - assert.strictEqual(shouldTransformCachedModuleCalls, 2); - assert.strictEqual(transformCalls, 3); + assert.strictEqual( + shouldTransformCachedModuleCalls, + 3, + 'final shouldTransformCachedModule calls' + ); + assert.strictEqual(transformCalls, 4, 'final transform calls'); assert.strictEqual(cachedModules[0].id, 'foo'); assert.deepStrictEqual(cachedModules[0].meta, { transform: { calls: 1, id: 'foo' } }); assert.strictEqual(cachedModules[1].id, 'entry'); - assert.deepStrictEqual(cachedModules[1].meta, { transform: { calls: 2, id: 'entry' } }); + assert.deepStrictEqual(cachedModules[1].meta, { transform: { calls: 3, id: 'entry' } }); + assert.strictEqual(cachedModules[2].id, 'bar'); + assert.deepStrictEqual(cachedModules[2].meta, { transform: { calls: 1, id: 'bar' } }); }); }); From f67c404d5411fecfca16ba87c3336305c4c80a77 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Thu, 17 Feb 2022 06:42:12 +0100 Subject: [PATCH 2/4] Test errors in shouldTransform are handled correctly --- .../plugin-error-should-transform/_config.js | 44 +++++++++++++++++++ .../plugin-error-should-transform/main.js | 1 + 2 files changed, 45 insertions(+) create mode 100644 test/function/samples/plugin-error-should-transform/_config.js create mode 100644 test/function/samples/plugin-error-should-transform/main.js diff --git a/test/function/samples/plugin-error-should-transform/_config.js b/test/function/samples/plugin-error-should-transform/_config.js new file mode 100644 index 00000000000..8f1780395b5 --- /dev/null +++ b/test/function/samples/plugin-error-should-transform/_config.js @@ -0,0 +1,44 @@ +const path = require('path'); +const acorn = require('acorn'); + +const code = 'export default 42;\n'; +const ID_MAIN = path.join(__dirname, 'main.js'); + +module.exports = { + description: 'errors in shouldTransformCachedModule abort the build', + options: { + cache: { + modules: [ + { + id: ID_MAIN, + ast: acorn.parse(code, { + ecmaVersion: 6, + sourceType: 'module' + }), + code, + dependencies: [], + dynamicDependencies: [], + originalCode: code, + resolvedIds: {}, + sourcemapChain: [], + transformDependencies: [] + } + ] + }, + plugins: [ + { + name: 'test', + shouldTransformCachedModule() { + throw new Error('broken'); + } + } + ] + }, + error: { + code: 'PLUGIN_ERROR', + hook: 'shouldTransformCachedModule', + message: 'broken', + plugin: 'test', + watchFiles: [ID_MAIN] + } +}; diff --git a/test/function/samples/plugin-error-should-transform/main.js b/test/function/samples/plugin-error-should-transform/main.js new file mode 100644 index 00000000000..7a4e8a723a4 --- /dev/null +++ b/test/function/samples/plugin-error-should-transform/main.js @@ -0,0 +1 @@ +export default 42; From ae21ad80e8821db05731ce66721ae46bc53c7a10 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 18 Feb 2022 06:46:15 +0100 Subject: [PATCH 3/4] Provide resolvedSources instead of importedIds --- src/Module.ts | 2 +- src/ModuleLoader.ts | 3 +-- src/rollup/types.d.ts | 6 ++---- test/incremental/index.js | 34 ++++++++++++++++++++-------------- 4 files changed, 24 insertions(+), 21 deletions(-) diff --git a/src/Module.ts b/src/Module.ts index 2810451ef8e..def8d4b330c 100644 --- a/src/Module.ts +++ b/src/Module.ts @@ -708,6 +708,7 @@ export default class Module { transformFiles, ...moduleOptions }: TransformModuleJSON & { + resolvedIds?: ResolvedIdMap; transformFiles?: EmittedFile[] | undefined; }): void { this.info.code = code; @@ -785,7 +786,6 @@ export default class Module { code: this.info.code!, customTransformCache: this.customTransformCache, dependencies: Array.from(this.dependencies, getId), - dynamicDependencies: Array.from(this.dynamicDependencies, getId), id: this.id, meta: this.info.meta, moduleSideEffects: this.info.moduleSideEffects, diff --git a/src/ModuleLoader.ts b/src/ModuleLoader.ts index 3cae7127a71..9589da12e51 100644 --- a/src/ModuleLoader.ts +++ b/src/ModuleLoader.ts @@ -280,11 +280,10 @@ export class ModuleLoader { { ast: cachedModule.ast, code: cachedModule.code, - dynamicallyImportedIds: cachedModule.dynamicDependencies, id: cachedModule.id, - importedIds: cachedModule.dependencies, meta: cachedModule.meta, moduleSideEffects: cachedModule.moduleSideEffects, + resolvedSources: cachedModule.resolvedIds, syntheticNamedExports: cachedModule.syntheticNamedExports } ])) diff --git a/src/rollup/types.d.ts b/src/rollup/types.d.ts index 8081dd07934..10a8341e873 100644 --- a/src/rollup/types.d.ts +++ b/src/rollup/types.d.ts @@ -108,7 +108,6 @@ export interface TransformModuleJSON { customTransformCache: boolean; originalCode: string; originalSourcemap: ExistingDecodedSourceMap | null; - resolvedIds?: ResolvedIdMap; sourcemapChain: DecodedSourceMapOrMissing[]; transformDependencies: string[]; } @@ -116,8 +115,8 @@ export interface TransformModuleJSON { export interface ModuleJSON extends TransformModuleJSON, ModuleOptions { ast: AcornNode; dependencies: string[]; - dynamicDependencies: string[]; id: string; + resolvedIds: ResolvedIdMap; transformFiles: EmittedFile[] | undefined; } @@ -252,11 +251,10 @@ export type ShouldTransformCachedModuleHook = ( options: { ast: AcornNode; code: string; - dynamicallyImportedIds: string[]; id: string; - importedIds: string[]; meta: CustomPluginOptions; moduleSideEffects: boolean | 'no-treeshake'; + resolvedSources: ResolvedIdMap; syntheticNamedExports: boolean | string; } ) => Promise | boolean; diff --git a/test/incremental/index.js b/test/incremental/index.js index 52458478ded..3d5b6bf01b9 100644 --- a/test/incremental/index.js +++ b/test/incremental/index.js @@ -346,14 +346,7 @@ describe('incremental', () => { let shouldTransformCachedModuleCalls = 0; const transformPlugin = { - async shouldTransformCachedModule({ - ast, - id, - meta, - importedIds, - dynamicallyImportedIds, - ...other - }) { + async shouldTransformCachedModule({ ast, id, meta, resolvedSources, ...other }) { shouldTransformCachedModuleCalls++; assert.strictEqual(ast.type, 'Program'); assert.deepStrictEqual(other, { @@ -364,19 +357,32 @@ describe('incremental', () => { switch (id) { case 'foo': assert.deepStrictEqual(meta, { transform: { calls: 1, id } }); - assert.deepStrictEqual(importedIds, []); - assert.deepStrictEqual(dynamicallyImportedIds, []); + assert.deepStrictEqual(resolvedSources, { __proto__: null }); // we return promises to ensure they are awaited return Promise.resolve(false); case 'bar': assert.deepStrictEqual(meta, { transform: { calls: 1, id } }); - assert.deepStrictEqual(importedIds, []); - assert.deepStrictEqual(dynamicallyImportedIds, []); + assert.deepStrictEqual(resolvedSources, { __proto__: null }); return Promise.resolve(false); case 'entry': assert.deepStrictEqual(meta, { transform: { calls: 0, id } }); - assert.deepStrictEqual(importedIds, ['foo']); - assert.deepStrictEqual(dynamicallyImportedIds, ['bar']); + assert.deepStrictEqual(resolvedSources, { + __proto__: null, + bar: { + external: false, + id: 'bar', + meta: {}, + moduleSideEffects: true, + syntheticNamedExports: false + }, + foo: { + external: false, + id: 'foo', + meta: {}, + moduleSideEffects: true, + syntheticNamedExports: false + } + }); return Promise.resolve(true); default: throw new Error(`Unexpected id ${id}.`); From 70a2b81ec8f4c3de108598c08e89e69e3cb4b896 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Sun, 20 Feb 2022 07:47:18 +0100 Subject: [PATCH 4/4] Add documentation --- docs/05-plugin-development.md | 2 +- test/incremental/index.js | 28 +++++++++++++++------------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/docs/05-plugin-development.md b/docs/05-plugin-development.md index 4b73a042072..c96f746ae6a 100644 --- a/docs/05-plugin-development.md +++ b/docs/05-plugin-development.md @@ -266,7 +266,7 @@ In watch mode or when using the cache explicitly, the resolved imports of a cach #### `shouldTransformCachedModule` -**Type:** `({id: string, code: string, ast: ESTree.Program, meta: {[plugin: string]: any}, moduleSideEffects: boolean | "no-treeshake", syntheticNamedExports: string | boolean}) => boolean`
**Kind:** `async, first`
**Previous Hook:** [`load`](guide/en/#load) where the cached file was loaded to compare its code with the cached version.
**Next Hook:** [`moduleParsed`](guide/en/#moduleparsed) if no plugin returns `true`, otherwise [`transform`](guide/en/#transform). +**Type:** `({id: string, code: string, ast: ESTree.Program, resoledSources: {[source: string]: ResolvedId}, meta: {[plugin: string]: any}, moduleSideEffects: boolean | "no-treeshake", syntheticNamedExports: string | boolean}) => boolean`
**Kind:** `async, first`
**Previous Hook:** [`load`](guide/en/#load) where the cached file was loaded to compare its code with the cached version.
**Next Hook:** [`moduleParsed`](guide/en/#moduleparsed) if no plugin returns `true`, otherwise [`transform`](guide/en/#transform). If the Rollup cache is used (e.g. in watch mode or explicitly via the JavaScript API), Rollup will skip the [`transform`](guide/en/#transform) hook of a module if after the [`load`](guide/en/#transform) hook, the loaded `code` is identical to the code of the cached copy. To prevent this, discard the cached copy and instead transform a module, plugins can implement this hook and return `true`. diff --git a/test/incremental/index.js b/test/incremental/index.js index 3d5b6bf01b9..542785ce873 100644 --- a/test/incremental/index.js +++ b/test/incremental/index.js @@ -339,9 +339,9 @@ describe('incremental', () => { it('runs shouldTransformCachedModule when using a cached module', async () => { modules = { - entry: `import foo from 'foo'; export default foo; import('bar').then(console.log);`, - foo: `export default 42`, - bar: `export default 21` + entry: `import foo from 'foo'; export default foo;`, + foo: `export default import('bar')`, + bar: `export default 42` }; let shouldTransformCachedModuleCalls = 0; @@ -357,24 +357,26 @@ describe('incremental', () => { switch (id) { case 'foo': assert.deepStrictEqual(meta, { transform: { calls: 1, id } }); - assert.deepStrictEqual(resolvedSources, { __proto__: null }); + assert.deepStrictEqual(resolvedSources, { + __proto__: null, + bar: { + external: false, + id: 'bar', + meta: {}, + moduleSideEffects: true, + syntheticNamedExports: false + } + }); // we return promises to ensure they are awaited return Promise.resolve(false); case 'bar': - assert.deepStrictEqual(meta, { transform: { calls: 1, id } }); + assert.deepStrictEqual(meta, { transform: { calls: 2, id } }); assert.deepStrictEqual(resolvedSources, { __proto__: null }); return Promise.resolve(false); case 'entry': assert.deepStrictEqual(meta, { transform: { calls: 0, id } }); assert.deepStrictEqual(resolvedSources, { __proto__: null, - bar: { - external: false, - id: 'bar', - meta: {}, - moduleSideEffects: true, - syntheticNamedExports: false - }, foo: { external: false, id: 'foo', @@ -421,6 +423,6 @@ describe('incremental', () => { assert.strictEqual(cachedModules[1].id, 'entry'); assert.deepStrictEqual(cachedModules[1].meta, { transform: { calls: 3, id: 'entry' } }); assert.strictEqual(cachedModules[2].id, 'bar'); - assert.deepStrictEqual(cachedModules[2].meta, { transform: { calls: 1, id: 'bar' } }); + assert.deepStrictEqual(cachedModules[2].meta, { transform: { calls: 2, id: 'bar' } }); }); });