Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] feat: per module inlineDynamicImport #3299

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/Module.ts
Expand Up @@ -194,6 +194,7 @@ export default class Module {
importDescriptions: { [name: string]: ImportDescription } = Object.create(null);
importMetas: MetaProperty[] = [];
imports = new Set<Variable>();
inlineDynamicImport = false;
isEntryPoint: boolean;
isExecuted = false;
isUserDefinedEntryPoint = false;
Expand Down Expand Up @@ -221,12 +222,19 @@ export default class Module {
private transformDependencies: string[] = [];
private transitiveReexports: string[] | null = null;

constructor(graph: Graph, id: string, moduleSideEffects: boolean, isEntry: boolean) {
constructor(
graph: Graph,
id: string,
moduleSideEffects: boolean,
inlineDynamicImport: boolean,
isEntry: boolean
) {
this.id = id;
this.graph = graph;
this.excludeFromSourcemap = /\0/.test(id);
this.context = graph.getModuleContext(id);
this.moduleSideEffects = moduleSideEffects;
this.inlineDynamicImport = inlineDynamicImport;
this.isEntryPoint = isEntry;
}

Expand Down Expand Up @@ -534,6 +542,7 @@ export default class Module {
originalSourcemap,
resolvedIds,
sourcemapChain,
inlineDynamicImport,
transformDependencies,
transformFiles
}: TransformModuleJSON & {
Expand All @@ -551,6 +560,9 @@ export default class Module {
if (typeof moduleSideEffects === 'boolean') {
this.moduleSideEffects = moduleSideEffects;
}
if (typeof inlineDynamicImport === 'boolean') {
this.inlineDynamicImport = inlineDynamicImport;
}

timeStart('generate ast', 3);

Expand Down Expand Up @@ -629,6 +641,7 @@ export default class Module {
customTransformCache: this.customTransformCache,
dependencies: this.dependencies.map(module => module.id),
id: this.id,
inlineDynamicImport: this.inlineDynamicImport,
moduleSideEffects: this.moduleSideEffects,
originalCode: this.originalCode,
originalSourcemap: this.originalSourcemap,
Expand Down
22 changes: 19 additions & 3 deletions src/ModuleLoader.ts
Expand Up @@ -272,6 +272,7 @@ export class ModuleLoader {
id: string,
importer: string,
moduleSideEffects: boolean,
inlineDynamicImport: boolean,
isEntry: boolean
): Promise<Module> {
const existingModule = this.modulesById.get(id);
Expand All @@ -280,7 +281,7 @@ export class ModuleLoader {
return Promise.resolve(existingModule);
}

const module: Module = new Module(this.graph, id, moduleSideEffects, isEntry);
const module = new Module(this.graph, id, moduleSideEffects, inlineDynamicImport, isEntry);
this.modulesById.set(id, module);
this.graph.watchFiles[id] = true;
const manualChunkAlias = this.getManualChunk(id);
Expand Down Expand Up @@ -321,6 +322,9 @@ export class ModuleLoader {
if (typeof sourceDescription.moduleSideEffects === 'boolean') {
module.moduleSideEffects = sourceDescription.moduleSideEffects;
}
if (typeof sourceDescription.inlineDynamicImport === 'boolean') {
module.inlineDynamicImport = sourceDescription.inlineDynamicImport;
}
return transform(this.graph, sourceDescription, module);
})
.then((source: TransformModuleJSON | ModuleJSON) => {
Expand Down Expand Up @@ -370,7 +374,13 @@ export class ModuleLoader {
}
return Promise.resolve(externalModule);
} else {
return this.fetchModule(resolvedId.id, importer, resolvedId.moduleSideEffects, false);
return this.fetchModule(
resolvedId.id,
importer,
resolvedId.moduleSideEffects,
resolvedId.inlineDynamicImport,
false
);
}
}

Expand All @@ -387,6 +397,7 @@ export class ModuleLoader {
return {
external: true,
id: source,
inlineDynamicImport: true,
moduleSideEffects: this.hasModuleSideEffects(source, true)
};
}
Expand All @@ -407,7 +418,7 @@ export class ModuleLoader {
: resolveIdResult;

if (typeof id === 'string') {
return this.fetchModule(id, undefined as any, true, isEntry);
return this.fetchModule(id, undefined as any, true, false, isEntry);
}
return error(errUnresolvedEntry(unresolvedId));
});
Expand All @@ -420,6 +431,7 @@ export class ModuleLoader {
let id = '';
let external = false;
let moduleSideEffects = null;
let inlineDynamicImport = false;
if (resolveIdResult) {
if (typeof resolveIdResult === 'object') {
id = resolveIdResult.id;
Expand All @@ -429,6 +441,9 @@ export class ModuleLoader {
if (typeof resolveIdResult.moduleSideEffects === 'boolean') {
moduleSideEffects = resolveIdResult.moduleSideEffects;
}
if (typeof resolveIdResult.inlineDynamicImport === 'boolean') {
inlineDynamicImport = resolveIdResult.inlineDynamicImport;
}
} else {
if (this.isExternal(resolveIdResult, importer, true)) {
external = true;
Expand All @@ -445,6 +460,7 @@ export class ModuleLoader {
return {
external,
id,
inlineDynamicImport,
moduleSideEffects:
typeof moduleSideEffects === 'boolean'
? moduleSideEffects
Expand Down
4 changes: 4 additions & 0 deletions src/rollup/types.d.ts
Expand Up @@ -91,6 +91,7 @@ export type SourceMapInput = ExistingRawSourceMap | string | null | { mappings:
export interface SourceDescription {
ast?: ESTree.Program;
code: string;
inlineDynamicImport?: boolean | null;
map?: SourceMapInput;
moduleSideEffects?: boolean | null;
}
Expand All @@ -104,6 +105,7 @@ export interface TransformModuleJSON {
code: string;
// note if plugins use new this.cache to opt-out auto transform cache
customTransformCache: boolean;
inlineDynamicImport: boolean | null;
moduleSideEffects: boolean | null;
originalCode: string;
originalSourcemap: ExistingDecodedSourceMap | null;
Expand Down Expand Up @@ -198,6 +200,7 @@ export interface PluginContextMeta {
export interface ResolvedId {
external: boolean;
id: string;
inlineDynamicImport: boolean;
moduleSideEffects: boolean;
}

Expand All @@ -208,6 +211,7 @@ export interface ResolvedIdMap {
interface PartialResolvedId {
external?: boolean;
id: string;
inlineDynamicImport?: boolean | null;
moduleSideEffects?: boolean | null;
}

Expand Down
6 changes: 6 additions & 0 deletions src/utils/chunkColouring.ts
Expand Up @@ -19,6 +19,11 @@ export function assignChunkColouringHashes(
Uint8ArrayXor(module.entryPointsHash, currentEntryHash);
}

for (const { resolution } of module.dynamicImports) {
if (resolution instanceof Module && resolution.inlineDynamicImport) {
module.dependencies.push(resolution);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is how we should do it. The reason is that module.dependencies is responsible for which imports the chunk has that this module ends up in. This means that if the target of the inlined dynamic import ends up in a different chunk than the import expression, this chunk will generate both the dynamic import as well as a static one. I added a test that show-cases this.

}
}
for (const dependency of module.dependencies) {
if (
dependency instanceof ExternalModule ||
Expand All @@ -35,6 +40,7 @@ export function assignChunkColouringHashes(
for (const { resolution } of module.dynamicImports) {
if (
resolution instanceof Module &&
!resolution.inlineDynamicImport &&
resolution.dynamicallyImportedBy.length > 0 &&
!resolution.manualChunkAlias
) {
Expand Down
5 changes: 5 additions & 0 deletions src/utils/transform.ts
Expand Up @@ -35,6 +35,7 @@ export default function transform(
const emittedFiles: EmittedFile[] = [];
let customTransformCache = false;
let moduleSideEffects: boolean | null = null;
let inlineDynamicImport: boolean | null = null;
let trackedPluginCache: { cache: PluginCache; used: boolean };
let curPlugin: Plugin;
const curSource: string = source.code;
Expand Down Expand Up @@ -82,6 +83,9 @@ export default function transform(
if (typeof result.moduleSideEffects === 'boolean') {
moduleSideEffects = result.moduleSideEffects;
}
if (typeof result.inlineDynamicImport === 'boolean') {
inlineDynamicImport = result.inlineDynamicImport;
}
} else {
return code;
}
Expand Down Expand Up @@ -189,6 +193,7 @@ export default function transform(
ast: ast as any,
code,
customTransformCache,
inlineDynamicImport,
moduleSideEffects,
originalCode,
originalSourcemap,
Expand Down
@@ -0,0 +1,16 @@
module.exports = {
description: 'inlines dynamic imports with side-effects that are only executed conditionally',
options: {
input: 'main.js',
plugins: {
transform(code, id) {
if (id.endsWith('inlined.js')) {
return {
code,
inlineDynamicImport: true
};
}
}
}
}
};
@@ -0,0 +1,17 @@
define(function () { 'use strict';

if (globalThis.unknown) {
Promise.resolve().then(function () { return inlined; }).then(console.log);
}

console.log('main1');

console.log('inlined');
const value = 'inlined';

var inlined = /*#__PURE__*/Object.freeze({
__proto__: null,
value: value
});

});
@@ -0,0 +1,15 @@
'use strict';

if (globalThis.unknown) {
Promise.resolve().then(function () { return inlined; }).then(console.log);
}

console.log('main1');

console.log('inlined');
const value = 'inlined';

var inlined = /*#__PURE__*/Object.freeze({
__proto__: null,
value: value
});
@@ -0,0 +1,13 @@
if (globalThis.unknown) {
Promise.resolve().then(function () { return inlined; }).then(console.log);
}

console.log('main1');

console.log('inlined');
const value = 'inlined';

var inlined = /*#__PURE__*/Object.freeze({
__proto__: null,
value: value
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the single biggest problem I encountered with the current implementation of this feature: The dynamic imports are no longer conditionally executed, they are immediately and synchronously executed. See this example: If globalThis.unknown is false, the namespace is not read, but the side-effect log statement of the dynamic import is still triggered. I am not sure this feature is valuable if this does not work. However I have no idea how to actually make it work. If we start wrapping code in conditionally executed function closures, then there would still be the problem if a dynamically imported module that is inlined has itself a static dependency that ends up in yet another chunk—how should this be handled? There are no conditional static imports.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it could technically be done by first dynamically importing the static dependencies that are in other chunks and THEN executing the conditionally imported code. But it would become quite a bit bigger than it is now because then we need to think about the whole chunk rendering process, not only the colouring algorithm. If we want to go there, I would suggest we should collect a few scenarios with expected sample code first before thinking about the implementation.

@@ -0,0 +1,22 @@
System.register([], function () {
'use strict';
return {
execute: function () {

if (globalThis.unknown) {
Promise.resolve().then(function () { return inlined; }).then(console.log);
}

console.log('main1');

console.log('inlined');
const value = 'inlined';

var inlined = /*#__PURE__*/Object.freeze({
__proto__: null,
value: value
});

}
};
});
@@ -0,0 +1,2 @@
console.log('inlined');
export const value = 'inlined';
@@ -0,0 +1,5 @@
if (globalThis.unknown) {
import('./inlined.js').then(console.log);
}

console.log('main1');
@@ -0,0 +1,36 @@
const fs = require('fs');
const path = require('path');

module.exports = {
description: 'inlines dynamic imports marked for inlining',
options: {
input: 'main.js',
plugins: {
resolveId(id, importer) {
if (id.endsWith('dep-inlined-via-resolveId.js')) {
return this.resolve(id, importer, { skipSelf: true }).then(resolution =>
Object.assign({}, resolution, { inlineDynamicImport: true })
);
}
},
load(id) {
if (id.endsWith('dep-inlined-via-load.js')) {
return {
code: fs.readFileSync(path.resolve(__dirname, 'dep-inlined-via-load.js'), {
encoding: 'utf8'
}),
inlineDynamicImport: true
};
}
},
transform(code, id) {
if (id.endsWith('dep-inlined-via-transform.js')) {
return {
code,
inlineDynamicImport: true
};
}
}
}
}
};
@@ -0,0 +1,8 @@
define(['exports'], function (exports) { 'use strict';

console.log('not inlined');
const value = 'not inlined';

exports.value = value;

});
@@ -0,0 +1,34 @@
define(['require'], function (require) { 'use strict';

Promise.resolve().then(function () { return depInlinedViaResolveId; }).then(console.log);
Promise.resolve().then(function () { return depInlinedViaLoad; }).then(console.log);
Promise.resolve().then(function () { return depInlinedViaTransform; }).then(console.log);
new Promise(function (resolve, reject) { require(['./generated-dep-not-inlined'], resolve, reject) }).then(console.log);

console.log('main');

console.log('resolveId');
const value = 'resolveId';

var depInlinedViaResolveId = /*#__PURE__*/Object.freeze({
__proto__: null,
value: value
});

console.log('load');
const value$1 = 'load';

var depInlinedViaLoad = /*#__PURE__*/Object.freeze({
__proto__: null,
value: value$1
});

console.log('transform');
const value$2 = 'transform';

var depInlinedViaTransform = /*#__PURE__*/Object.freeze({
__proto__: null,
value: value$2
});

});