Skip to content

Commit

Permalink
Refine namespace conflict handling to ignore and warn for known confl…
Browse files Browse the repository at this point in the history 
…icts
  • Loading branch information
@lukastaegert
lukastaegert committed May 3, 2021
1 parent 7e30b81 commit 3c031cf
Show file tree
Hide file tree
Showing 66 changed files with 906 additions and 134 deletions.
36 changes: 15 additions & 21 deletions cli/run/batchWarnings.ts
View file
@@ -1,6 +1,7 @@
import { bold, gray, yellow } from 'colorette';
import { RollupWarning } from '../../src/rollup/types';
import { getOrCreate } from '../../src/utils/getOrCreate';
import { printQuotedStringList } from '../../src/utils/printStringList';
import relativeId from '../../src/utils/relativeId';
import { stderr } from '../logging';

Expand Down Expand Up @@ -79,15 +80,10 @@ const immediateHandlers: {
MISSING_NODE_BUILTINS: warning => {
title(`Missing shims for Node.js built-ins`);

const detail =
warning.modules!.length === 1
? `'${warning.modules![0]}'`
: `${warning
.modules!.slice(0, -1)
.map((name: string) => `'${name}'`)
.join(', ')} and '${warning.modules!.slice(-1)}'`;
stderr(
`Creating a browser bundle that depends on ${detail}. You might need to include https://github.com/ionic-team/rollup-plugin-node-polyfills`
`Creating a browser bundle that depends on ${printQuotedStringList(
warning.modules!
)}. You might need to include https://github.com/ionic-team/rollup-plugin-node-polyfills`
);
}
};
Expand Down Expand Up @@ -162,11 +158,11 @@ const deferredHandlers: {
title(`Conflicting re-exports`);
for (const warning of warnings) {
stderr(
`${bold(relativeId(warning.reexporter!))} re-exports '${
`"${bold(relativeId(warning.reexporter!))}" re-exports "${
warning.name
}' from both ${relativeId(warning.sources![0])} and ${relativeId(
}" from both "${relativeId(warning.sources![0])}" and "${relativeId(
warning.sources![1]
)} (will be ignored)`
)}" (will be ignored)`
);
}
},
Expand Down Expand Up @@ -207,16 +203,14 @@ const deferredHandlers: {
title(`Broken sourcemap`);
info('https://rollupjs.org/guide/en/#warning-sourcemap-is-likely-to-be-incorrect');

const plugins = Array.from(new Set(warnings.map(w => w.plugin).filter(Boolean)));
const detail =
plugins.length > 1
? ` (such as ${plugins
.slice(0, -1)
.map(p => `'${p}'`)
.join(', ')} and '${plugins.slice(-1)}')`
: ` (such as '${plugins[0]}')`;

stderr(`Plugins that transform code${detail} should generate accompanying sourcemaps`);
const plugins = [
...new Set(warnings.map(warning => warning.plugin).filter(Boolean))
] as string[];
stderr(
`Plugins that transform code (such as ${printQuotedStringList(
plugins
)}) should generate accompanying sourcemaps`
);
},

THIS_IS_UNDEFINED(warnings) {
Expand Down
31 changes: 12 additions & 19 deletions src/ExternalModule.ts
View file
Expand Up @@ -8,6 +8,8 @@ import {
import { EMPTY_ARRAY } from './utils/blank';
import { makeLegal } from './utils/identifierHelpers';
import { normalize, relative } from './utils/path';
import { printQuotedStringList } from './utils/printStringList';
import relativeId from './utils/relativeId';

export default class ExternalModule {
chunk: void;
Expand Down Expand Up @@ -99,32 +101,23 @@ export default class ExternalModule {
const declaration = this.declarations[name];
return !declaration.included && !this.reexported && !declaration.referenced;
});

if (unused.length === 0) return;

const names =
unused.length === 1
? `'${unused[0]}' is`
: `${unused
.slice(0, -1)
.map(name => `'${name}'`)
.join(', ')} and '${unused.slice(-1)}' are`;

const importersSet = new Set<string>();
for (const name of unused) {
const { importers, dynamicImporters } = this.declarations[name].module;

if (Array.isArray(importers)) importers.forEach(v => importersSet.add(v));
if (Array.isArray(dynamicImporters)) dynamicImporters.forEach(v => importersSet.add(v));
const { importers } = this.declarations[name].module;
for (const importer of importers) {
importersSet.add(importer);
}
}

const importersArray = Array.from(importersSet);

const importerList = ' in' + importersArray.map(s => `\n\t${s};`);

const importersArray = [...importersSet];
this.options.onwarn({
code: 'UNUSED_EXTERNAL_IMPORT',
message: `${names} imported from external module '${this.id}' but never used${importerList}`,
message: `${printQuotedStringList(unused, ['is', 'are'])} imported from external module "${
this.id
}" but never used in ${printQuotedStringList(
importersArray.map(importer => relativeId(importer))
)}.`,
names: unused,
source: this.id,
sources: importersArray
Expand Down
111 changes: 79 additions & 32 deletions src/Module.ts
View file
Expand Up @@ -48,6 +48,7 @@ import {
import { EMPTY_OBJECT } from './utils/blank';
import {
augmentCodeLocation,
errAmbiguousExternalNamespaces,
errCircularReexport,
errMissingExport,
errNamespaceConflict,
Expand Down Expand Up @@ -258,6 +259,7 @@ export default class Module {
private exportNamesByVariable: Map<Variable, string[]> | null = null;
private exportShimVariable: ExportShimVariable = new ExportShimVariable(this);
private magicString!: MagicString;
private namespaceReexportsByName: Record<string, Variable | null> = Object.create(null);
private relevantDependencies: Set<Module | ExternalModule> | null = null;
private syntheticExports = new Map<string, SyntheticNamedExportVariable>();
private syntheticNamespace: Variable | null | undefined = null;
Expand Down Expand Up @@ -369,7 +371,10 @@ export default class Module {
) {
dependencyVariables = new Set(dependencyVariables);
for (const exportName of [...this.getReexports(), ...this.getExports()]) {
dependencyVariables.add(this.getVariableForExportName(exportName)!);
const exportedVariable = this.getVariableForExportName(exportName);
if (exportedVariable) {
dependencyVariables.add(exportedVariable);
}
}
}
for (let variable of dependencyVariables) {
Expand Down Expand Up @@ -553,33 +558,17 @@ export default class Module {
}

if (name !== 'default') {
let foundSyntheticDeclaration: SyntheticNamedExportVariable | null = null;
const skipExternalNamespaceValues = new Set([true, skipExternalNamespaceReexports]);
for (const skipExternalNamespaces of skipExternalNamespaceValues) {
for (const module of this.exportAllModules) {
if (module instanceof Module || !skipExternalNamespaces) {
const declaration = getVariableForExportNameRecursive(
module,
const foundNamespaceReexport =
name in this.namespaceReexportsByName
? this.namespaceReexportsByName[name]
: (this.namespaceReexportsByName[name] = this.getVariableFromNamespaceReexports(
name,
importerForSideEffects,
true,
searchedNamesAndModules,
skipExternalNamespaces
);

if (declaration) {
if (!(declaration instanceof SyntheticNamedExportVariable)) {
return declaration;
}
if (!foundSyntheticDeclaration) {
foundSyntheticDeclaration = declaration;
}
}
}
}
}
if (foundSyntheticDeclaration) {
return foundSyntheticDeclaration;
skipExternalNamespaceReexports
));
if (foundNamespaceReexport) {
return foundNamespaceReexport;
}
}

Expand Down Expand Up @@ -638,13 +627,15 @@ export default class Module {
}

for (const name of this.getReexports()) {
const variable = this.getVariableForExportName(name)!;
variable.deoptimizePath(UNKNOWN_PATH);
if (!variable.included) {
this.includeVariable(variable);
}
if (variable instanceof ExternalVariable) {
variable.module.reexported = true;
const variable = this.getVariableForExportName(name);
if (variable) {
variable.deoptimizePath(UNKNOWN_PATH);
if (!variable.included) {
this.includeVariable(variable);
}
if (variable instanceof ExternalVariable) {
variable.module.reexported = true;
}
}
}

Expand Down Expand Up @@ -1050,6 +1041,62 @@ export default class Module {
addSideEffectDependencies(alwaysCheckedDependencies);
}

private getVariableFromNamespaceReexports(
name: string,
importerForSideEffects?: Module,
searchedNamesAndModules?: Map<string, Set<Module | ExternalModule>>,
skipExternalNamespaceReexports = false
): Variable | null {
let foundSyntheticDeclaration: SyntheticNamedExportVariable | null = null;
const skipExternalNamespaceValues = new Set([true, skipExternalNamespaceReexports]);
for (const skipExternalNamespaces of skipExternalNamespaceValues) {
const foundDeclarations = new Set<Variable>();
for (const module of this.exportAllModules) {
if (module instanceof Module || !skipExternalNamespaces) {
const declaration = getVariableForExportNameRecursive(
module,
name,
importerForSideEffects,
true,
searchedNamesAndModules,
skipExternalNamespaces
);

if (declaration) {
if (!(declaration instanceof SyntheticNamedExportVariable)) {
foundDeclarations.add(declaration);
} else if (!foundSyntheticDeclaration) {
foundSyntheticDeclaration = declaration;
}
}
}
}
if (foundDeclarations.size === 1) {
return [...foundDeclarations][0];
}
if (foundDeclarations.size > 1) {
if (skipExternalNamespaces) {
return null;
}
const foundDeclarationList = [...(foundDeclarations as Set<ExternalVariable>)];
const usedDeclaration = foundDeclarationList[0];
this.options.onwarn(
errAmbiguousExternalNamespaces(
name,
this.id,
usedDeclaration.module.id,
foundDeclarationList.map(declaration => declaration.module.id)
)
);
return usedDeclaration;
}
}
if (foundSyntheticDeclaration) {
return foundSyntheticDeclaration;
}
return null;
}

private includeAndGetAdditionalMergedNamespaces(): Variable[] {
const mergedNamespaces: Variable[] = [];
for (const module of this.exportAllModules) {
Expand Down
7 changes: 5 additions & 2 deletions src/ast/variables/NamespaceVariable.ts
View file
Expand Up @@ -44,10 +44,13 @@ export default class NamespaceVariable extends Variable {
if (this.memberVariables) {
return this.memberVariables;
}
const memberVariables = Object.create(null);
const memberVariables: { [name: string]: Variable } = Object.create(null);
for (const name of this.context.getExports().concat(this.context.getReexports())) {
if (name[0] !== '*' && name !== this.module.info.syntheticNamedExports) {
memberVariables[name] = this.context.traceExport(name);
const exportedVariable = this.context.traceExport(name);
if (exportedVariable) {
memberVariables[name] = exportedVariable;
}
}
}
return (this.memberVariables = memberVariables);
Expand Down
13 changes: 4 additions & 9 deletions src/finalisers/shared/warnOnBuiltins.ts
View file
@@ -1,5 +1,6 @@
import { ChunkDependencies } from '../../Chunk';
import { RollupWarning } from '../../rollup/types';
import { printQuotedStringList } from '../../utils/printStringList';

const builtins = {
assert: true,
Expand Down Expand Up @@ -33,17 +34,11 @@ export default function warnOnBuiltins(

if (!externalBuiltins.length) return;

const detail =
externalBuiltins.length === 1
? `module ('${externalBuiltins[0]}')`
: `modules (${externalBuiltins
.slice(0, -1)
.map(name => `'${name}'`)
.join(', ')} and '${externalBuiltins.slice(-1)}')`;

warn({
code: 'MISSING_NODE_BUILTINS',
message: `Creating a browser bundle that depends on Node.js built-in ${detail}. You might need to include https://github.com/ionic-team/rollup-plugin-node-polyfills`,
message: `Creating a browser bundle that depends on Node.js built-in modules (${printQuotedStringList(
externalBuiltins
)}). You might need to include https://github.com/ionic-team/rollup-plugin-node-polyfills`,
modules: externalBuiltins
});
}
39 changes: 29 additions & 10 deletions src/utils/error.ts
View file
Expand Up @@ -8,6 +8,7 @@ import {
WarningHandler
} from '../rollup/types';
import getCodeFrame from './getCodeFrame';
import { printQuotedStringList } from './printStringList';
import relativeId from './relativeId';

export function error(base: Error | RollupError): never {
Expand Down Expand Up @@ -63,6 +64,7 @@ export const enum Errors {
MISSING_IMPLICIT_DEPENDANT = 'MISSING_IMPLICIT_DEPENDANT',
MIXED_EXPORTS = 'MIXED_EXPORTS',
NAMESPACE_CONFLICT = 'NAMESPACE_CONFLICT',
AMBIGUOUS_EXTERNAL_NAMESPACES = 'AMBIGUOUS_EXTERNAL_NAMESPACES',
NO_TRANSFORM_MAP_OR_AST_WITHOUT_CODE = 'NO_TRANSFORM_MAP_OR_AST_WITHOUT_CODE',
PLUGIN_ERROR = 'PLUGIN_ERROR',
PREFER_NAMED_EXPORTS = 'PREFER_NAMED_EXPORTS',
Expand Down Expand Up @@ -307,13 +309,11 @@ export function errImplicitDependantIsNotIncluded(module: Module) {
).sort();
return {
code: Errors.MISSING_IMPLICIT_DEPENDANT,
message: `Module "${relativeId(module.id)}" that should be implicitly loaded before "${
implicitDependencies.length === 1
? implicitDependencies[0]
: `${implicitDependencies.slice(0, -1).join('", "')}" and "${
implicitDependencies.slice(-1)[0]
}`
}" is not included in the module graph. Either it was not imported by an included module or only via a tree-shaken dynamic import, or no imported bindings were used and it had otherwise no side-effects.`
message: `Module "${relativeId(
module.id
)}" that should be implicitly loaded before ${printQuotedStringList(
implicitDependencies
)} is not included in the module graph. Either it was not imported by an included module or only via a tree-shaken dynamic import, or no imported bindings were used and it had otherwise no side-effects.`
};
}

Expand All @@ -337,17 +337,36 @@ export function errNamespaceConflict(
) {
return {
code: Errors.NAMESPACE_CONFLICT,
message: `Conflicting namespaces: ${relativeId(
message: `Conflicting namespaces: "${relativeId(
reexportingModule.id
)} re-exports '${name}' from both ${relativeId(
)}" re-exports "${name}" from both "${relativeId(
reexportingModule.exportsAll[name]
)} and ${relativeId(additionalExportAllModule.exportsAll[name])} (will be ignored)`,
)}" and "${relativeId(additionalExportAllModule.exportsAll[name])}" (will be ignored)`,
name,
reexporter: reexportingModule.id,
sources: [reexportingModule.exportsAll[name], additionalExportAllModule.exportsAll[name]]
};
}

export function errAmbiguousExternalNamespaces(
name: string,
reexportingModule: string,
usedExternalModule: string,
externalModules: string[]
) {
return {
code: Errors.AMBIGUOUS_EXTERNAL_NAMESPACES,
message: `Ambiguous external namespace resolution: "${relativeId(
reexportingModule
)}" re-exports "${name}" from one of the external modules ${printQuotedStringList(
externalModules.map(module => relativeId(module))
)}, guessing "${relativeId(usedExternalModule)}".`,
name,
reexporter: reexportingModule,
sources: externalModules
};
}

export function errNoTransformMapOrAstWithoutCode(pluginName: string) {
return {
code: Errors.NO_TRANSFORM_MAP_OR_AST_WITHOUT_CODE,
Expand Down

0 comments on commit 3c031cf

Please sign in to comment.