Skip to content

Commit

Permalink
Avoid dynamic facade chunks (#3535)
Browse files Browse the repository at this point in the history 
* Always create namespace variables

* Clean up import expression resolution

* Implement dynamic import facade chunk prevention

* Support compact mode

* Handle importing entries with strict, tainted and missing facades

* Do not run CJS dynamic imports synchronously

* Improve coverage
  • Loading branch information
@lukastaegert
lukastaegert committed May 6, 2020
1 parent 468010b commit f514c76
Show file tree
Hide file tree
Showing 215 changed files with 1,000 additions and 459 deletions.
38 changes: 38 additions & 0 deletions scripts/update-snapshots.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
const { readdirSync, copydirSync, copyFileSync, rimrafSync } = require('sander');
const { resolve, join } = require('path');

const basePath = resolve(__dirname, '../test');

const formPath = join(basePath, 'form/samples');
const formDirsToHandle = readdirSync(formPath);
for (const dir of formDirsToHandle) {
const testPath = join(formPath, dir);
const testFiles = readdirSync(testPath);
if (!testFiles.includes('_config.js')) {
formDirsToHandle.push(...testFiles.map(filename => join(dir, filename)));
} else if (testFiles.includes('_actual')) {
const expectedPath = join(testPath, '_expected');
rimrafSync(expectedPath);
copydirSync(join(testPath, '_actual')).to(expectedPath);
} else if (testFiles.includes('_actual.js')) {
copyFileSync(join(testPath, '_actual.js')).to(join(testPath, '_expected.js'));
} else {
throw new Error(`Could not find test output in ${testPath}`);
}
}

const chunkingPath = join(basePath, 'chunking-form/samples');
const chunkingDirsToHandle = readdirSync(chunkingPath);
for (const dir of chunkingDirsToHandle) {
const testPath = join(chunkingPath, dir);
const testFiles = readdirSync(testPath);
if (!testFiles.includes('_config.js')) {
chunkingDirsToHandle.push(...testFiles.map(filename => join(dir, filename)));
} else if (testFiles.includes('_actual')) {
const expectedPath = join(testPath, '_expected');
rimrafSync(expectedPath);
copydirSync(join(testPath, '_actual')).to(expectedPath);
} else {
throw new Error(`Could not find test output in ${testPath}`);
}
}
118 changes: 79 additions & 39 deletions src/Chunk.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ export default class Chunk {
}
chunk.dependencies.add(facadedModule.chunk!);
chunk.facadeModule = facadedModule;
chunk.strictFacade = true;
return chunk;
}

Expand All @@ -138,6 +139,7 @@ export default class Chunk {
graph: Graph;
id: string | null = null;
indentString: string = undefined as any;
isDynamicEntry = false;
manualChunkAlias: string | null = null;
orderedModules: Module[];
renderedModules?: {
Expand All @@ -148,6 +150,7 @@ export default class Chunk {

private dependencies = new Set<ExternalModule | Chunk>();
private dynamicDependencies = new Set<ExternalModule | Chunk>();
private dynamicEntryModules: Module[] = [];
private exports = new Set<Variable>();
private exportsByName: Record<string, Variable> = Object.create(null);
private fileName: string | null = null;
Expand All @@ -164,6 +167,7 @@ export default class Chunk {
private renderedModuleSources = new Map<Module, MagicString>();
private renderedSource: MagicStringBundle | null = null;
private sortedExportNames: string[] | null = null;
private strictFacade = false;

constructor(graph: Graph, orderedModules: Module[]) {
this.graph = graph;
Expand All @@ -178,16 +182,18 @@ export default class Chunk {
this.manualChunkAlias = module.manualChunkAlias;
}
module.chunk = this;
if (
module.isEntryPoint ||
module.dynamicallyImportedBy.some(module => orderedModules.indexOf(module) === -1)
) {
if (module.isEntryPoint) {
this.entryModules.push(module);
}
if (module.dynamicallyImportedBy.length > 0) {
this.dynamicEntryModules.push(module);
}
}

const moduleForNaming =
this.entryModules[0] || this.orderedModules[this.orderedModules.length - 1];
this.entryModules[0] ||
this.dynamicEntryModules[0] ||
this.orderedModules[this.orderedModules.length - 1];
if (moduleForNaming) {
this.variableName = makeLegal(
basename(
Expand All @@ -199,7 +205,7 @@ export default class Chunk {
}
}

canModuleBeFacade(module: Module): boolean {
canModuleBeFacade(module: Module, exposedNamespaces: NamespaceVariable[]): boolean {
const moduleExportNamesByVariable = module.getExportNamesByVariable();
for (const exposedVariable of this.exports) {
if (!moduleExportNamesByVariable.has(exposedVariable)) {
Expand All @@ -221,6 +227,13 @@ export default class Chunk {
return false;
}
}
for (const exposedVariable of exposedNamespaces) {
if (
!(moduleExportNamesByVariable.has(exposedVariable) || exposedVariable.module === module)
) {
return false;
}
}
return true;
}

Expand All @@ -230,8 +243,7 @@ export default class Chunk {
const remainingExports = new Set(this.exports);
if (
this.facadeModule !== null &&
(this.facadeModule.preserveSignature !== false ||
this.facadeModule.dynamicallyImportedBy.some(importer => importer.chunk !== this))
(this.facadeModule.preserveSignature !== false || this.strictFacade)
) {
const exportNamesByVariable = this.facadeModule.getExportNamesByVariable();
for (const [variable, exportNames] of exportNamesByVariable) {
Expand All @@ -254,6 +266,11 @@ export default class Chunk {

generateFacades(): Chunk[] {
const facades: Chunk[] = [];
const dynamicEntryModules = this.dynamicEntryModules.filter(module =>
module.dynamicallyImportedBy.some(importingModule => importingModule.chunk !== this)
);
this.isDynamicEntry = dynamicEntryModules.length > 0;
const exposedNamespaces = dynamicEntryModules.map(module => module.namespace);
for (const module of this.entryModules) {
const requiredFacades: FacadeName[] = Array.from(module.userChunkNames).map(name => ({
name
Expand All @@ -265,22 +282,39 @@ export default class Chunk {
if (requiredFacades.length === 0) {
requiredFacades.push({});
}
if (!this.facadeModule) {
if (
this.graph.preserveModules ||
(module.preserveSignature !== 'strict' && !module.dynamicallyImportedBy.length) ||
this.canModuleBeFacade(module)
) {
this.facadeModule = module;
module.facadeChunk = this;
this.assignFacadeName(requiredFacades.shift()!, module);
}
if (
!this.facadeModule &&
(this.graph.preserveModules ||
module.preserveSignature !== 'strict' ||
this.canModuleBeFacade(module, exposedNamespaces))
) {
this.facadeModule = module;
module.facadeChunk = this;
this.strictFacade = module.preserveSignature === 'strict';
this.assignFacadeName(requiredFacades.shift()!, module);
}

for (const facadeName of requiredFacades) {
facades.push(Chunk.generateFacade(this.graph, module, facadeName));
}
}
for (const module of dynamicEntryModules) {
if (!this.facadeModule && this.canModuleBeFacade(module, exposedNamespaces)) {
this.facadeModule = module;
module.facadeChunk = this;
this.strictFacade = true;
this.assignFacadeName({}, module);
} else if (
this.facadeModule === module &&
!this.strictFacade &&
this.canModuleBeFacade(module, exposedNamespaces)
) {
this.strictFacade = true;
} else if (!(module.facadeChunk && module.facadeChunk.strictFacade)) {
module.namespace.include();
this.exports.add(module.namespace);
}
}
return facades;
}

Expand Down Expand Up @@ -465,7 +499,7 @@ export default class Chunk {
magicString.addSource(source);
this.usedModules.push(module);
}
const namespace = module.getOrCreateNamespace();
const namespace = module.namespace;
if (namespace.included && !this.graph.preserveModules) {
const rendered = namespace.renderBlock(renderOptions);
if (namespace.renderFirst()) hoistedSource += n + rendered;
Expand Down Expand Up @@ -545,7 +579,7 @@ export default class Chunk {
renderedDependency.id = this.getRelativePath(depId, false);
}

this.finaliseDynamicImports(format === 'amd');
this.finaliseDynamicImports(options);
this.finaliseImportMetas(format, outputPluginDriver);

const hasExports =
Expand Down Expand Up @@ -694,7 +728,8 @@ export default class Chunk {
return hash.digest('hex').substr(0, 8);
}

private finaliseDynamicImports(stripKnownJsExtensions: boolean) {
private finaliseDynamicImports(options: OutputOptions) {
const stripKnownJsExtensions = options.format === 'amd';
for (const [module, code] of this.renderedModuleSources) {
for (const { node, resolution } of module.dynamicImports) {
if (
Expand All @@ -706,15 +741,25 @@ export default class Chunk {
}
const renderedResolution =
resolution instanceof Module
? `'${this.getRelativePath(resolution.facadeChunk!.id!, stripKnownJsExtensions)}'`
? `'${this.getRelativePath(
(resolution.facadeChunk || resolution.chunk!).id!,
stripKnownJsExtensions
)}'`
: resolution instanceof ExternalModule
? `'${
resolution.renormalizeRenderPath
? this.getRelativePath(resolution.renderPath, stripKnownJsExtensions)
: resolution.renderPath
}'`
: resolution;
node.renderFinalResolution(code, renderedResolution);
node.renderFinalResolution(
code,
renderedResolution,
resolution instanceof Module &&
!(resolution.facadeChunk && resolution.facadeChunk.strictFacade) &&
resolution.namespace.exportName!,
options
);
}
}
}
Expand Down Expand Up @@ -893,7 +938,7 @@ export default class Chunk {
exports: this.getExportNames(),
facadeModuleId: facadeModule && facadeModule.id,
imports: this.getImportIds(),
isDynamicEntry: facadeModule !== null && facadeModule.dynamicallyImportedBy.length > 0,
isDynamicEntry: this.isDynamicEntry,
isEntry: facadeModule !== null && facadeModule.isEntryPoint,
modules: this.renderedModules!,
get name() {
Expand All @@ -913,14 +958,8 @@ export default class Chunk {
private inlineChunkDependencies(chunk: Chunk) {
for (const dep of chunk.dependencies) {
if (this.dependencies.has(dep)) continue;
if (dep instanceof ExternalModule) {
this.dependencies.add(dep);
} else {
// At the moment, circular dependencies between chunks are not possible; this will
// change if we ever add logic to ensure correct execution order or open up the
// chunking to plugins
// if (dep === this) continue;
this.dependencies.add(dep);
this.dependencies.add(dep);
if (dep instanceof Chunk) {
this.inlineChunkDependencies(dep);
}
}
Expand All @@ -932,13 +971,12 @@ export default class Chunk {
if (!node.included) continue;
if (resolution instanceof Module) {
if (resolution.chunk === this) {
const namespace = resolution.getOrCreateNamespace();
node.setResolution('named', resolution, namespace);
node.setInternalResolution(resolution.namespace);
} else {
node.setResolution(resolution.chunk!.exportMode, resolution);
node.setExternalResolution(resolution.chunk!.exportMode, resolution);
}
} else {
node.setResolution('auto', resolution);
node.setExternalResolution('auto', resolution);
}
}
}
Expand Down Expand Up @@ -1026,7 +1064,9 @@ export default class Chunk {
) {
const map = module.getExportNamesByVariable();
for (const exportedVariable of map.keys()) {
this.exports.add(exportedVariable);
if (module.isEntryPoint && module.preserveSignature !== false) {
this.exports.add(exportedVariable);
}
const isSynthetic = exportedVariable instanceof SyntheticNamedExportVariable;
const importedVariable = isSynthetic
? (exportedVariable as SyntheticNamedExportVariable).getBaseVariable()
Expand All @@ -1045,7 +1085,7 @@ export default class Chunk {
}
}
}
if (module.getOrCreateNamespace().included) {
if (module.namespace.included) {
for (const reexportName of Object.keys(module.reexportDescriptions)) {
const reexport = module.reexportDescriptions[reexportName];
const variable = reexport.module.getVariableForExportName(reexport.localName);
Expand All @@ -1057,7 +1097,7 @@ export default class Chunk {
}
for (const { node, resolution } of module.dynamicImports) {
if (node.included && resolution instanceof Module && resolution.chunk === this)
resolution.getOrCreateNamespace().include();
resolution.namespace.include();
}
}
}
17 changes: 5 additions & 12 deletions src/Module.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ export default class Module {
isExecuted = false;
isUserDefinedEntryPoint = false;
manualChunkAlias: string = null as any;
namespace!: NamespaceVariable;
originalCode!: string;
originalSourcemap!: ExistingDecodedSourceMap | null;
preserveSignature: PreserveEntrySignaturesOption = this.graph.preserveEntrySignatures ?? 'strict';
Expand All @@ -238,7 +239,6 @@ export default class Module {
private exportNamesByVariable: Map<Variable, string[]> | null = null;
private exportShimVariable: ExportShimVariable = new ExportShimVariable(this);
private magicString!: MagicString;
private namespaceVariable: NamespaceVariable | null = null;
private relevantDependencies: Set<Module | ExternalModule> | null = null;
private syntheticExports = new Map<string, SyntheticNamedExportVariable>();
private transformDependencies: string[] = [];
Expand Down Expand Up @@ -432,14 +432,6 @@ export default class Module {
return Object.keys(this.exports);
}

getOrCreateNamespace(): NamespaceVariable {
if (!this.namespaceVariable) {
this.namespaceVariable = new NamespaceVariable(this.astContext, this.syntheticNamedExports);
this.namespaceVariable.initialise();
}
return this.namespaceVariable;
}

getReexports(): string[] {
if (this.transitiveReexports) {
return this.transitiveReexports;
Expand Down Expand Up @@ -481,7 +473,7 @@ export default class Module {
): Variable {
if (name[0] === '*') {
if (name.length === 1) {
return this.getOrCreateNamespace();
return this.namespace;
} else {
// export * from 'external'
const module = this.graph.moduleById.get(name.slice(1)) as ExternalModule;
Expand Down Expand Up @@ -593,7 +585,7 @@ export default class Module {
}

isIncluded() {
return this.ast.included || (this.namespaceVariable && this.namespaceVariable.included);
return this.ast.included || this.namespace.included;
}

linkDependencies() {
Expand Down Expand Up @@ -734,6 +726,7 @@ export default class Module {
};

this.scope = new ModuleScope(this.graph.scope, this.astContext);
this.namespace = new NamespaceVariable(this.astContext, this.syntheticNamedExports);
this.ast = new Program(
this.esTreeAst,
{ type: 'Module', context: this.astContext },
Expand Down Expand Up @@ -773,7 +766,7 @@ export default class Module {
const otherModule = importDeclaration.module;

if (otherModule instanceof Module && importDeclaration.name === '*') {
return otherModule.getOrCreateNamespace();
return otherModule.namespace;
}

const declaration = otherModule.getVariableForExportName(importDeclaration.name);
Expand Down

0 comments on commit f514c76

Please sign in to comment.