From 95aa3b16b53a4d7796a6f791c4fead166bebbbaa Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Sun, 29 Jan 2023 10:09:05 +0100 Subject: [PATCH 1/2] Improve and fix cross-chunk-reexport warning --- src/Chunk.ts | 12 ++-- src/utils/error.ts | 7 +- .../circular-missed-reexports/_config.js | 8 +++ .../_config.js | 67 +++++++++++++++++++ .../formatters.js | 3 + .../index.js | 2 + .../main.js | 2 + .../types.js | 2 + .../_config.js | 44 ++++++++++++ .../formatters.js | 3 + .../index.js | 2 + .../main.js | 2 + .../types.js | 2 + .../circular-preserve-modules/_config.js | 6 +- 14 files changed, 153 insertions(+), 9 deletions(-) create mode 100644 test/function/samples/circular-namespace-reexport-manual-chunks/_config.js create mode 100644 test/function/samples/circular-namespace-reexport-manual-chunks/formatters.js create mode 100644 test/function/samples/circular-namespace-reexport-manual-chunks/index.js create mode 100644 test/function/samples/circular-namespace-reexport-manual-chunks/main.js create mode 100644 test/function/samples/circular-namespace-reexport-manual-chunks/types.js create mode 100644 test/function/samples/circular-namespace-reexport-preserve-modules/_config.js create mode 100644 test/function/samples/circular-namespace-reexport-preserve-modules/formatters.js create mode 100644 test/function/samples/circular-namespace-reexport-preserve-modules/index.js create mode 100644 test/function/samples/circular-namespace-reexport-preserve-modules/main.js create mode 100644 test/function/samples/circular-namespace-reexport-preserve-modules/types.js diff --git a/src/Chunk.ts b/src/Chunk.ts index 2ef889da1f6..c5486a8f7ad 100644 --- a/src/Chunk.ts +++ b/src/Chunk.ts @@ -710,13 +710,15 @@ export default class Chunk { alternativeReexportModule = importingModule.alternativeReexportModules.get(variable); if (alternativeReexportModule) { const exportingChunk = this.chunkByModule.get(alternativeReexportModule); - if (exportingChunk && exportingChunk !== exportChunk) { + if (exportingChunk !== exportChunk) { this.inputOptions.onwarn( errorCyclicCrossChunkReexport( - variableModule.getExportNamesByVariable().get(variable)![0], + // Namespaces do not have an export name + variableModule.getExportNamesByVariable().get(variable)?.[0] || '*', variableModule.id, alternativeReexportModule.id, - importingModule.id + importingModule.id, + this.outputOptions.preserveModules ) ); } @@ -732,8 +734,10 @@ export default class Chunk { for (const exportedVariable of map.keys()) { const isSynthetic = exportedVariable instanceof SyntheticNamedExportVariable; const importedVariable = isSynthetic ? exportedVariable.getBaseVariable() : exportedVariable; + this.checkCircularDependencyImport(importedVariable, module); + // When preserving modules, we do not create namespace objects but directly + // use the actual namespaces, which would be broken by this logic. if (!(importedVariable instanceof NamespaceVariable && this.outputOptions.preserveModules)) { - this.checkCircularDependencyImport(importedVariable, module); const exportingModule = importedVariable.module; if (exportingModule instanceof Module) { const chunk = this.chunkByModule.get(exportingModule); diff --git a/src/utils/error.ts b/src/utils/error.ts index 0448d6ad8b1..2a7d4a12a72 100644 --- a/src/utils/error.ts +++ b/src/utils/error.ts @@ -259,7 +259,8 @@ export function errorCyclicCrossChunkReexport( exportName: string, exporter: string, reexporter: string, - importer: string + importer: string, + preserveModules: boolean ): RollupLog { return { code: CYCLIC_CROSS_CHUNK_REEXPORT, @@ -271,7 +272,9 @@ export function errorCyclicCrossChunkReexport( reexporter )}" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "${relativeId( importer - )}" to point directly to the exporting module or do not use "preserveModules" to ensure these modules end up in the same chunk.`, + )}" to point directly to the exporting module or ${ + preserveModules ? 'do not use "output.preserveModules"' : 'reconfigure "output.manualChunks"' + } to ensure these modules end up in the same chunk.`, reexporter }; } diff --git a/test/function/samples/circular-missed-reexports/_config.js b/test/function/samples/circular-missed-reexports/_config.js index 64eee42d4ec..727d8069c4a 100644 --- a/test/function/samples/circular-missed-reexports/_config.js +++ b/test/function/samples/circular-missed-reexports/_config.js @@ -38,6 +38,14 @@ module.exports = { ^ 2: export { exists };`, url: 'https://rollupjs.org/troubleshooting/#error-name-is-not-exported-by-module' + }, + { + code: 'CYCLIC_CROSS_CHUNK_REEXPORT', + exporter: ID_DEP1, + id: ID_MAIN, + message: + 'Export "exists4" of module "dep1.js" was reexported through module "dep2.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "main.js" to point directly to the exporting module or reconfigure "output.manualChunks" to ensure these modules end up in the same chunk.', + reexporter: ID_DEP2 } ] }; diff --git a/test/function/samples/circular-namespace-reexport-manual-chunks/_config.js b/test/function/samples/circular-namespace-reexport-manual-chunks/_config.js new file mode 100644 index 00000000000..eff91817d4c --- /dev/null +++ b/test/function/samples/circular-namespace-reexport-manual-chunks/_config.js @@ -0,0 +1,67 @@ +const path = require('node:path'); + +const ID_INDEX = path.join(__dirname, 'index.js'); +const ID_TYPES = path.join(__dirname, 'types.js'); +const ID_FORMATTERS = path.join(__dirname, 'formatters.js'); +const ID_MAIN = path.join(__dirname, 'main.js'); + +module.exports = { + description: + 'correctly handles namespace reexports with circular dependencies when using manual chunks', + options: { + output: { + manualChunks(id) { + return path.basename(id); + } + } + }, + warnings: [ + { + code: 'CIRCULAR_DEPENDENCY', + ids: [ID_INDEX, ID_TYPES, ID_INDEX], + message: 'Circular dependency: index.js -> types.js -> index.js' + }, + { + code: 'CIRCULAR_DEPENDENCY', + ids: [ID_INDEX, ID_FORMATTERS, ID_INDEX], + message: 'Circular dependency: index.js -> formatters.js -> index.js' + }, + { + code: 'CYCLIC_CROSS_CHUNK_REEXPORT', + exporter: ID_TYPES, + id: ID_FORMATTERS, + message: + 'Export "LANGUAGES" of module "types.js" was reexported through module "index.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "formatters.js" to point directly to the exporting module or reconfigure "output.manualChunks" to ensure these modules end up in the same chunk.', + reexporter: ID_INDEX + }, + { + code: 'CYCLIC_CROSS_CHUNK_REEXPORT', + exporter: ID_FORMATTERS, + id: ID_MAIN, + message: + 'Export "*" of module "formatters.js" was reexported through module "index.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "main.js" to point directly to the exporting module or reconfigure "output.manualChunks" to ensure these modules end up in the same chunk.', + reexporter: ID_INDEX + }, + { + code: 'CYCLIC_CROSS_CHUNK_REEXPORT', + exporter: ID_FORMATTERS, + id: ID_MAIN, + message: + 'Export "*" of module "formatters.js" was reexported through module "index.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "main.js" to point directly to the exporting module or reconfigure "output.manualChunks" to ensure these modules end up in the same chunk.', + reexporter: ID_INDEX + }, + { + code: 'CYCLIC_CROSS_CHUNK_REEXPORT', + exporter: ID_FORMATTERS, + id: ID_TYPES, + message: + 'Export "*" of module "formatters.js" was reexported through module "index.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "types.js" to point directly to the exporting module or reconfigure "output.manualChunks" to ensure these modules end up in the same chunk.', + reexporter: ID_INDEX + }, + { + code: 'EMPTY_BUNDLE', + message: 'Generated an empty chunk: "index.js".', + names: ['index.js'] + } + ] +}; diff --git a/test/function/samples/circular-namespace-reexport-manual-chunks/formatters.js b/test/function/samples/circular-namespace-reexport-manual-chunks/formatters.js new file mode 100644 index 00000000000..202cbf4161d --- /dev/null +++ b/test/function/samples/circular-namespace-reexport-manual-chunks/formatters.js @@ -0,0 +1,3 @@ +import { LANGUAGES } from './index.js'; + +export const format = () => LANGUAGES; diff --git a/test/function/samples/circular-namespace-reexport-manual-chunks/index.js b/test/function/samples/circular-namespace-reexport-manual-chunks/index.js new file mode 100644 index 00000000000..26d94d9e3c9 --- /dev/null +++ b/test/function/samples/circular-namespace-reexport-manual-chunks/index.js @@ -0,0 +1,2 @@ +export { LANGUAGES } from './types.js'; +export * as formatters from './formatters.js'; diff --git a/test/function/samples/circular-namespace-reexport-manual-chunks/main.js b/test/function/samples/circular-namespace-reexport-manual-chunks/main.js new file mode 100644 index 00000000000..ab8895256dd --- /dev/null +++ b/test/function/samples/circular-namespace-reexport-manual-chunks/main.js @@ -0,0 +1,2 @@ +import { formatters } from './index.js' +export default formatters; diff --git a/test/function/samples/circular-namespace-reexport-manual-chunks/types.js b/test/function/samples/circular-namespace-reexport-manual-chunks/types.js new file mode 100644 index 00000000000..2773e071ac9 --- /dev/null +++ b/test/function/samples/circular-namespace-reexport-manual-chunks/types.js @@ -0,0 +1,2 @@ +import { formatters } from './index.js' +export const LANGUAGES = () => formatters; diff --git a/test/function/samples/circular-namespace-reexport-preserve-modules/_config.js b/test/function/samples/circular-namespace-reexport-preserve-modules/_config.js new file mode 100644 index 00000000000..34a4a905bc0 --- /dev/null +++ b/test/function/samples/circular-namespace-reexport-preserve-modules/_config.js @@ -0,0 +1,44 @@ +const path = require('node:path'); + +const ID_INDEX = path.join(__dirname, 'index.js'); +const ID_TYPES = path.join(__dirname, 'types.js'); +const ID_FORMATTERS = path.join(__dirname, 'formatters.js'); +const ID_MAIN = path.join(__dirname, 'main.js'); + +module.exports = { + description: + 'correctly handles namespace reexports with circular dependencies when preserving modules', + options: { + output: { + preserveModules: true + } + }, + warnings: [ + { + code: 'CIRCULAR_DEPENDENCY', + ids: [ID_INDEX, ID_TYPES, ID_INDEX], + message: 'Circular dependency: index.js -> types.js -> index.js' + }, + { + code: 'CIRCULAR_DEPENDENCY', + ids: [ID_INDEX, ID_FORMATTERS, ID_INDEX], + message: 'Circular dependency: index.js -> formatters.js -> index.js' + }, + { + code: 'CYCLIC_CROSS_CHUNK_REEXPORT', + exporter: ID_FORMATTERS, + id: ID_MAIN, + message: + 'Export "*" of module "formatters.js" was reexported through module "index.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "main.js" to point directly to the exporting module or do not use "output.preserveModules" to ensure these modules end up in the same chunk.', + reexporter: ID_INDEX + }, + { + code: 'CYCLIC_CROSS_CHUNK_REEXPORT', + exporter: ID_TYPES, + id: ID_FORMATTERS, + message: + 'Export "LANGUAGES" of module "types.js" was reexported through module "index.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "formatters.js" to point directly to the exporting module or do not use "output.preserveModules" to ensure these modules end up in the same chunk.', + reexporter: ID_INDEX + } + ] +}; diff --git a/test/function/samples/circular-namespace-reexport-preserve-modules/formatters.js b/test/function/samples/circular-namespace-reexport-preserve-modules/formatters.js new file mode 100644 index 00000000000..202cbf4161d --- /dev/null +++ b/test/function/samples/circular-namespace-reexport-preserve-modules/formatters.js @@ -0,0 +1,3 @@ +import { LANGUAGES } from './index.js'; + +export const format = () => LANGUAGES; diff --git a/test/function/samples/circular-namespace-reexport-preserve-modules/index.js b/test/function/samples/circular-namespace-reexport-preserve-modules/index.js new file mode 100644 index 00000000000..26d94d9e3c9 --- /dev/null +++ b/test/function/samples/circular-namespace-reexport-preserve-modules/index.js @@ -0,0 +1,2 @@ +export { LANGUAGES } from './types.js'; +export * as formatters from './formatters.js'; diff --git a/test/function/samples/circular-namespace-reexport-preserve-modules/main.js b/test/function/samples/circular-namespace-reexport-preserve-modules/main.js new file mode 100644 index 00000000000..ab8895256dd --- /dev/null +++ b/test/function/samples/circular-namespace-reexport-preserve-modules/main.js @@ -0,0 +1,2 @@ +import { formatters } from './index.js' +export default formatters; diff --git a/test/function/samples/circular-namespace-reexport-preserve-modules/types.js b/test/function/samples/circular-namespace-reexport-preserve-modules/types.js new file mode 100644 index 00000000000..2773e071ac9 --- /dev/null +++ b/test/function/samples/circular-namespace-reexport-preserve-modules/types.js @@ -0,0 +1,2 @@ +import { formatters } from './index.js' +export const LANGUAGES = () => formatters; diff --git a/test/function/samples/circular-preserve-modules/_config.js b/test/function/samples/circular-preserve-modules/_config.js index 27c2c49981e..ee8b3ded422 100644 --- a/test/function/samples/circular-preserve-modules/_config.js +++ b/test/function/samples/circular-preserve-modules/_config.js @@ -4,7 +4,7 @@ const ID_FIRST = path.join(__dirname, 'first.js'); const ID_SECOND = path.join(__dirname, 'second.js'); module.exports = { - description: 'correctly handles circular dependencies whe preserving modules', + description: 'correctly handles circular dependencies when preserving modules', options: { output: { preserveModules: true } }, @@ -24,7 +24,7 @@ module.exports = { exporter: ID_SECOND, id: ID_FIRST, message: - 'Export "second" of module "second.js" was reexported through module "main.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "first.js" to point directly to the exporting module or do not use "preserveModules" to ensure these modules end up in the same chunk.', + 'Export "second" of module "second.js" was reexported through module "main.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "first.js" to point directly to the exporting module or do not use "output.preserveModules" to ensure these modules end up in the same chunk.', reexporter: ID_MAIN }, { @@ -32,7 +32,7 @@ module.exports = { exporter: ID_FIRST, id: ID_SECOND, message: - 'Export "first" of module "first.js" was reexported through module "main.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "second.js" to point directly to the exporting module or do not use "preserveModules" to ensure these modules end up in the same chunk.', + 'Export "first" of module "first.js" was reexported through module "main.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "second.js" to point directly to the exporting module or do not use "output.preserveModules" to ensure these modules end up in the same chunk.', reexporter: ID_MAIN } ] From e440b4d91bba583f4a07b6bf898bbc4b244b620e Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Mon, 30 Jan 2023 06:35:24 +0100 Subject: [PATCH 2/2] Show warning also for namespace imports --- src/Chunk.ts | 11 ++++++----- .../_config.js | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/Chunk.ts b/src/Chunk.ts index c5486a8f7ad..21f65ec3eb1 100644 --- a/src/Chunk.ts +++ b/src/Chunk.ts @@ -1378,12 +1378,13 @@ export default class Chunk { const chunk = this.chunkByModule.get(variable.module as Module); if (chunk !== this) { this.imports.add(variable); - if ( - !(variable instanceof NamespaceVariable && this.outputOptions.preserveModules) && - variable.module instanceof Module - ) { - chunk!.exports.add(variable); + if (variable.module instanceof Module) { this.checkCircularDependencyImport(variable, module); + // When preserving modules, we do not create namespace objects but directly + // use the actual namespaces, which would be broken by this logic. + if (!(variable instanceof NamespaceVariable && this.outputOptions.preserveModules)) { + chunk!.exports.add(variable); + } } } } diff --git a/test/function/samples/circular-namespace-reexport-preserve-modules/_config.js b/test/function/samples/circular-namespace-reexport-preserve-modules/_config.js index 34a4a905bc0..a66f558f911 100644 --- a/test/function/samples/circular-namespace-reexport-preserve-modules/_config.js +++ b/test/function/samples/circular-namespace-reexport-preserve-modules/_config.js @@ -32,6 +32,22 @@ module.exports = { 'Export "*" of module "formatters.js" was reexported through module "index.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "main.js" to point directly to the exporting module or do not use "output.preserveModules" to ensure these modules end up in the same chunk.', reexporter: ID_INDEX }, + { + code: 'CYCLIC_CROSS_CHUNK_REEXPORT', + exporter: ID_FORMATTERS, + id: ID_MAIN, + message: + 'Export "*" of module "formatters.js" was reexported through module "index.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "main.js" to point directly to the exporting module or do not use "output.preserveModules" to ensure these modules end up in the same chunk.', + reexporter: ID_INDEX + }, + { + code: 'CYCLIC_CROSS_CHUNK_REEXPORT', + exporter: ID_FORMATTERS, + id: ID_TYPES, + message: + 'Export "*" of module "formatters.js" was reexported through module "index.js" while both modules are dependencies of each other and will end up in different chunks by current Rollup settings. This scenario is not well supported at the moment as it will produce a circular dependency between chunks and will likely lead to broken execution order.\nEither change the import in "types.js" to point directly to the exporting module or do not use "output.preserveModules" to ensure these modules end up in the same chunk.', + reexporter: ID_INDEX + }, { code: 'CYCLIC_CROSS_CHUNK_REEXPORT', exporter: ID_TYPES,