Skip to content

Commit 55e7e2d

Browse files
lukastaegertlukastaegert
committed
feat(commonjs): auto-detect conditional requires (#1038)
1 parent 38a3aa4 commit 55e7e2d

File tree

18 files changed

+364
-65
lines changed

18 files changed

+364
-65
lines changed

‎packages/commonjs/src/generate-imports.js

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ export function getRequireHandlers() {
104104
scope,
105105
usesReturnValue,
106106
isInsideTryBlock,
107+
isInsideConditional,
107108
toBeRemoved
108109
) {
109110
requireExpressions.push({
@@ -112,6 +113,7 @@ export function getRequireHandlers() {
112113
scope,
113114
usesReturnValue,
114115
isInsideTryBlock,
116+
isInsideConditional,
115117
toBeRemoved
116118
});
117119
}
@@ -135,7 +137,6 @@ export function getRequireHandlers() {
135137
const imports = [];
136138
imports.push(`import * as ${helpersName} from "${HELPERS_ID}";`);
137139
if (usesRequire) {
138-
// TODO Lukas check where to import it from or change to usesDynamicRequire
139140
imports.push(
140141
`import { commonjsRequire as ${dynamicRequireName} } from "${DYNAMIC_MODULES_ID}";`
141142
);
@@ -155,7 +156,12 @@ export function getRequireHandlers() {
155156
const { requireTargets, usesRequireWrapper } = await resolveRequireSourcesAndGetMeta(
156157
id,
157158
needsRequireWrapper ? IS_WRAPPED_COMMONJS : !isEsModule,
158-
Object.keys(requiresBySource)
159+
Object.keys(requiresBySource).map((source) => {
160+
return {
161+
source,
162+
isConditional: requiresBySource[source].every((require) => require.isInsideConditional)
163+
};
164+
})
159165
);
160166
processRequireExpressions(
161167
imports,

‎packages/commonjs/src/index.js

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export default function commonjs(options = {}) {
3838
} = options;
3939
const extensions = options.extensions || ['.js'];
4040
const filter = createFilter(options.include, options.exclude);
41-
const { strictRequiresFilter, detectCycles } = getStrictRequiresFilter(options);
41+
const { strictRequiresFilter, detectCyclesAndConditional } = getStrictRequiresFilter(options);
4242

4343
const getRequireReturnsDefault =
4444
typeof requireReturnsDefaultOption === 'function'
@@ -63,10 +63,11 @@ export default function commonjs(options = {}) {
6363
resolveRequireSourcesAndGetMeta,
6464
getWrappedIds,
6565
isRequiredId
66-
} = getResolveRequireSourcesAndGetMeta(extensions, detectCycles);
66+
} = getResolveRequireSourcesAndGetMeta(extensions, detectCyclesAndConditional);
6767
const dynamicRequireModules = getDynamicRequireModules(options.dynamicRequireTargets);
6868
const isDynamicRequireModulesEnabled = dynamicRequireModules.size > 0;
69-
// TODO Lukas do we need the CWD?
69+
// TODO Lukas replace with new dynamicRequireRoot to replace CWD
70+
// TODO Lukas throw if require from outside commondir
7071
const commonDir = isDynamicRequireModulesEnabled
7172
? getCommonDir(null, Array.from(dynamicRequireModules.keys()).concat(process.cwd()))
7273
: null;

‎packages/commonjs/src/resolve-require-sources.js

Lines changed: 32 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,10 @@ import {
77
} from './helpers';
88
import { resolveExtensions } from './resolve-id';
99

10-
export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
10+
export function getResolveRequireSourcesAndGetMeta(extensions, detectCyclesAndConditional) {
1111
const knownCjsModuleTypes = Object.create(null);
1212
const requiredIds = Object.create(null);
13+
const unconditionallyRequiredIds = Object.create(null);
1314
const dependentModules = Object.create(null);
1415
const getDependentModules = (id) =>
1516
dependentModules[id] || (dependentModules[id] = Object.create(null));
@@ -20,20 +21,31 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
2021
(id) => knownCjsModuleTypes[id] === IS_WRAPPED_COMMONJS
2122
),
2223
isRequiredId: (id) => requiredIds[id],
23-
resolveRequireSourcesAndGetMeta: (rollupContext) => async (id, isParentCommonJS, sources) => {
24-
knownCjsModuleTypes[id] = isParentCommonJS;
24+
resolveRequireSourcesAndGetMeta: (rollupContext) => async (
25+
parentId,
26+
isParentCommonJS,
27+
sources
28+
) => {
29+
knownCjsModuleTypes[parentId] = knownCjsModuleTypes[parentId] || isParentCommonJS;
30+
if (
31+
knownCjsModuleTypes[parentId] &&
32+
requiredIds[parentId] &&
33+
!unconditionallyRequiredIds[parentId]
34+
) {
35+
knownCjsModuleTypes[parentId] = IS_WRAPPED_COMMONJS;
36+
}
2537
const requireTargets = await Promise.all(
26-
sources.map(async (source) => {
38+
sources.map(async ({ source, isConditional }) => {
2739
// Never analyze or proxy internal modules
2840
if (source.startsWith('\0')) {
2941
return { id: source, allowProxy: false };
3042
}
3143
const resolved =
32-
(await rollupContext.resolve(source, id, {
44+
(await rollupContext.resolve(source, parentId, {
3345
custom: {
3446
'node-resolve': { isRequire: true }
3547
}
36-
})) || resolveExtensions(source, id, extensions);
48+
})) || resolveExtensions(source, parentId, extensions);
3749
if (!resolved) {
3850
return { id: wrapId(source, EXTERNAL_SUFFIX), allowProxy: false };
3951
}
@@ -42,17 +54,25 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
4254
return { id: wrapId(childId, EXTERNAL_SUFFIX), allowProxy: false };
4355
}
4456
requiredIds[childId] = true;
45-
const parentDependentModules = getDependentModules(id);
57+
if (
58+
!(
59+
detectCyclesAndConditional &&
60+
(isConditional || knownCjsModuleTypes[parentId] === IS_WRAPPED_COMMONJS)
61+
)
62+
) {
63+
unconditionallyRequiredIds[childId] = true;
64+
}
65+
const parentDependentModules = getDependentModules(parentId);
4666
const childDependentModules = getDependentModules(childId);
47-
childDependentModules[id] = true;
67+
childDependentModules[parentId] = true;
4868
for (const dependentId of Object.keys(parentDependentModules)) {
4969
childDependentModules[dependentId] = true;
5070
}
5171
if (parentDependentModules[childId]) {
5272
// If we depend on one of our dependencies, we have a cycle. Then all modules that
5373
// we depend on that also depend on the same module are part of a cycle as well.
54-
if (detectCycles && isParentCommonJS) {
55-
knownCjsModuleTypes[id] = IS_WRAPPED_COMMONJS;
74+
if (detectCyclesAndConditional && isParentCommonJS) {
75+
knownCjsModuleTypes[parentId] = IS_WRAPPED_COMMONJS;
5676
knownCjsModuleTypes[childId] = IS_WRAPPED_COMMONJS;
5777
for (const dependentId of Object.keys(parentDependentModules)) {
5878
if (getDependentModules(dependentId)[childId]) {
@@ -73,7 +93,7 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
7393
requireTargets: requireTargets.map(({ id: dependencyId, allowProxy }, index) => {
7494
const isCommonJS = knownCjsModuleTypes[dependencyId];
7595
return {
76-
source: sources[index],
96+
source: sources[index].source,
7797
id: allowProxy
7898
? isCommonJS === IS_WRAPPED_COMMONJS
7999
? wrapId(dependencyId, WRAPPED_SUFFIX)
@@ -82,7 +102,7 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
82102
isCommonJS
83103
};
84104
}),
85-
usesRequireWrapper: knownCjsModuleTypes[id] === IS_WRAPPED_COMMONJS
105+
usesRequireWrapper: knownCjsModuleTypes[parentId] === IS_WRAPPED_COMMONJS
86106
};
87107
}
88108
};

‎packages/commonjs/src/transform-commonjs.js

Lines changed: 54 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,13 @@ export default async function transformCommonjs(
7171
let shouldWrap = false;
7272

7373
const globals = new Set();
74+
// A conditionalNode is a node for which execution is not guaranteed. If such a node is a require
75+
// or contains nested requires, those should be handled as function calls unless there is an
76+
// unconditional require elsewhere.
77+
let currentConditionalNodeEnd = null;
78+
const conditionalNodes = new Set();
7479

80+
// TODO Lukas fix this at last, we are close
7581
// TODO technically wrong since globals isn't populated yet, but ¯\_(ツ)_/¯
7682
const helpersName = deconflict([scope], globals, 'commonjsHelpers');
7783
const dynamicRequireName = deconflict([scope], globals, 'commonjsRequire');
@@ -102,6 +108,12 @@ export default async function transformCommonjs(
102108
if (currentTryBlockEnd !== null && node.start > currentTryBlockEnd) {
103109
currentTryBlockEnd = null;
104110
}
111+
if (currentConditionalNodeEnd !== null && node.start > currentConditionalNodeEnd) {
112+
currentConditionalNodeEnd = null;
113+
}
114+
if (currentConditionalNodeEnd === null && conditionalNodes.has(node)) {
115+
currentConditionalNodeEnd = node.end;
116+
}
105117

106118
programDepth += 1;
107119
if (node.scope) ({ scope } = node);
@@ -113,11 +125,6 @@ export default async function transformCommonjs(
113125

114126
// eslint-disable-next-line default-case
115127
switch (node.type) {
116-
case 'TryStatement':
117-
if (currentTryBlockEnd === null) {
118-
currentTryBlockEnd = node.block.end;
119-
}
120-
return;
121128
case 'AssignmentExpression':
122129
if (node.left.type === 'MemberExpression') {
123130
const flattened = getKeypath(node.left);
@@ -227,6 +234,7 @@ export default async function transformCommonjs(
227234
scope,
228235
usesReturnValue,
229236
currentTryBlockEnd !== null,
237+
currentConditionalNodeEnd !== null,
230238
parent.type === 'ExpressionStatement' ? parent : node
231239
);
232240
}
@@ -237,8 +245,26 @@ export default async function transformCommonjs(
237245
// skip dead branches
238246
if (isFalsy(node.test)) {
239247
skippedNodes.add(node.consequent);
240-
} else if (node.alternate && isTruthy(node.test)) {
241-
skippedNodes.add(node.alternate);
248+
} else if (isTruthy(node.test)) {
249+
if (node.alternate) {
250+
skippedNodes.add(node.alternate);
251+
}
252+
} else {
253+
conditionalNodes.add(node.consequent);
254+
if (node.alternate) {
255+
conditionalNodes.add(node.alternate);
256+
}
257+
}
258+
return;
259+
case 'ArrowFunctionExpression':
260+
case 'FunctionDeclaration':
261+
case 'FunctionExpression':
262+
// requires in functions should be conditional unless it is an IIFE
263+
if (
264+
currentConditionalNodeEnd === null &&
265+
!(parent.type === 'CallExpression' && parent.callee === node)
266+
) {
267+
currentConditionalNodeEnd = node.end;
242268
}
243269
return;
244270
case 'Identifier': {
@@ -301,6 +327,22 @@ export default async function transformCommonjs(
301327
return;
302328
}
303329
}
330+
case 'LogicalExpression':
331+
// skip dead branches
332+
if (node.operator === '&&') {
333+
if (isFalsy(node.left)) {
334+
skippedNodes.add(node.right);
335+
} else if (!isTruthy(node.left)) {
336+
conditionalNodes.add(node.right);
337+
}
338+
} else if (node.operator === '||') {
339+
if (isTruthy(node.left)) {
340+
skippedNodes.add(node.right);
341+
} else if (!isFalsy(node.left)) {
342+
conditionalNodes.add(node.right);
343+
}
344+
}
345+
return;
304346
case 'MemberExpression':
305347
if (!isDynamicRequireModulesEnabled && isModuleRequire(node, scope)) {
306348
uses.require = true;
@@ -328,6 +370,11 @@ export default async function transformCommonjs(
328370
}
329371
}
330372
return;
373+
case 'TryStatement':
374+
if (currentTryBlockEnd === null) {
375+
currentTryBlockEnd = node.block.end;
376+
}
377+
return;
331378
case 'UnaryExpression':
332379
// rewrite `typeof module`, `typeof module.exports` and `typeof exports` (https://github.com/rollup/rollup-plugin-commonjs/issues/151)
333380
if (node.operator === 'typeof') {

‎packages/commonjs/src/utils.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,20 +44,20 @@ export function capitalize(name) {
4444
export function getStrictRequiresFilter({ strictRequires }) {
4545
switch (strictRequires) {
4646
case true:
47-
return { strictRequiresFilter: () => true, detectCycles: false };
47+
return { strictRequiresFilter: () => true, detectCyclesAndConditional: false };
4848
// eslint-disable-next-line no-undefined
4949
case undefined:
5050
case 'auto':
5151
case 'debug':
5252
case null:
53-
return { strictRequiresFilter: () => false, detectCycles: true };
53+
return { strictRequiresFilter: () => false, detectCyclesAndConditional: true };
5454
case false:
55-
return { strictRequiresFilter: () => false, detectCycles: false };
55+
return { strictRequiresFilter: () => false, detectCyclesAndConditional: false };
5656
default:
5757
if (typeof strictRequires === 'string' || Array.isArray(strictRequires)) {
5858
return {
5959
strictRequiresFilter: createFilter(strictRequires),
60-
detectCycles: false
60+
detectCyclesAndConditional: false
6161
};
6262
}
6363
throw new Error('Unexpected value for "strictRequires" option.');

‎packages/commonjs/test/fixtures/function/conditional-require-chain/_config.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
module.exports = {
2+
description: 'makes requires in conditionally required modules conditional as well'
3+
};

‎packages/commonjs/test/fixtures/function/conditional-require-chain/dep.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
require('./throws.js');

‎packages/commonjs/test/fixtures/function/conditional-require-chain/main.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
global.false = false;
2+
3+
if (global.false) {
4+
// eslint-disable-next-line global-require
5+
require('./dep.js');
6+
}

‎packages/commonjs/test/fixtures/function/conditional-require-chain/throws.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
throw new Error('This should not be executed');

‎packages/commonjs/test/fixtures/function/skips-dead-branches/main.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,9 @@ if ('development' === 'production') {
33
require('./a.js');
44
}
55

6-
module.exports = true ? require('./b.js') : require('./c.js');
6+
exports.conditionalTrue = true ? require('./b.js') : require('./c.js');
7+
exports.conditionalFalse = false ? require('./c.js') : require('./b.js');
8+
exports.logicalAnd1 = true && require('./b.js');
9+
exports.logicalAnd2 = false && require('./c.js');
10+
exports.logicalOr1 = true || require('./c.js');
11+
exports.logicalOr2 = false || require('./b.js');

0 commit comments

Comments
 (0)