From 480b80f71f5030536368d220cd7e754b22d696ef Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:28:33 +0100 Subject: [PATCH 1/3] Inline requires: Fix memoization order and partial rewrites With `memoizeCalls`, destructured requires sorted their references by source position to decide which one assigns the memoized module. Nodes inserted by earlier plugins have no position, so an `exports.a = a` added by the import-export plugin could take the assignment while an earlier `a()` read the unassigned module. References are now sorted in program order, as `scope.crawl()` collects them. Both the identifier and destructured paths swallowed `replaceWith` errors, eg for references in type annotations or `export {...}` specifiers, leaving a mix of inlined and original references. References are now checked up front with Babel's `validate`, and a require with any reference that can't be rewritten is left alone. The memo `var` is added to the program body directly, since `addStmtToBlock` can't fail for a `Program`. Changelog: [Fix] Inline requires: Don't partially inline requires referenced in type annotations or `export {...}` specifiers --- .../__tests__/inline-requires-plugin-test.js | 75 ++++++ .../src/inline-requires-plugin.js | 214 ++++++++++-------- 2 files changed, 195 insertions(+), 94 deletions(-) diff --git a/packages/metro-transform-plugins/src/__tests__/inline-requires-plugin-test.js b/packages/metro-transform-plugins/src/__tests__/inline-requires-plugin-test.js index 3b1c13a26f..cf1b47bf49 100644 --- a/packages/metro-transform-plugins/src/__tests__/inline-requires-plugin-test.js +++ b/packages/metro-transform-plugins/src/__tests__/inline-requires-plugin-test.js @@ -15,10 +15,12 @@ import type {Plugins} from '@babel/core'; import type {Node as BabelNode} from '@babel/types'; import type {PluginTesterOptions} from 'babel-plugin-tester'; +import importExportPlugin from '../import-export-plugin'; import inlineRequiresPlugin from '../inline-requires-plugin'; import validateOutputAst from './validateOutputAst'; import * as babel from '@babel/core'; import pluginTester from 'babel-plugin-tester'; +import vm from 'node:vm'; import nullthrows from 'nullthrows'; type TestCases = PluginTesterOptions['tests']; @@ -325,6 +327,22 @@ const TEST_CASES: TestCases = { snapshot: true, }, + 'ignores requires exported by name': { + code: ['var foo = require("foo");', 'foo();', 'export { foo };'].join('\n'), + snapshot: false, + }, + + 'ignores destructured requires exported by name': { + code: [ + 'const {', + ' a', + '} = require("foo");', + 'a();', + 'export { a };', + ].join('\n'), + snapshot: false, + }, + 'ignores destructured requires in `ignoredRequires`': { code: ['const {', ' a', '} = require("CommonFoo");', 'a();'].join('\n'), snapshot: false, @@ -348,6 +366,63 @@ describe.each([true, false])('memoizeCalls=%s:', memoizeCalls => { }); describe('inline-requires', () => { + test.each([[false], [true]])( + 'ignores requires referenced in type annotations (memoizeCalls=%s)', + memoizeCalls => { + const source = [ + 'const foo = require("foo");', + 'const {a} = require("bar");', + 'type T = typeof foo | typeof a;', + 'foo(a);', + ].join('\n'); + const transformFlow = (plugins: Plugins) => + babel.transformSync(source, { + babelrc: false, + configFile: false, + parserOpts: {plugins: ['flow']}, + plugins, + })?.code; + expect(transformFlow([[inlineRequiresPlugin, {memoizeCalls}]])).toBe( + transformFlow([]), + ); + }, + ); + + test.each([[false], [true]])( + 'inlines destructured requires exported by the import-export plugin (memoizeCalls=%s)', + memoizeCalls => { + // The import-export plugin runs first in the same pass and inserts + // `exports.a = a` after `a()`, with no source location. + const code = nullthrows( + babel.transformSync( + ['const {a} = require("foo");', 'a();', 'export {a};'].join('\n'), + { + babelrc: false, + configFile: false, + sourceType: 'module', + plugins: [ + [ + importExportPlugin, + { + importAll: '_$$_IMPORT_ALL', + importDefault: '_$$_IMPORT_DEFAULT', + resolve: false, + }, + ], + [inlineRequiresPlugin, {memoizeCalls}], + ], + }, + )?.code, + ); + expect(code).not.toMatch(/const \{/); + const foo = {a: jest.fn()}; + const exports: {a?: unknown} = {}; + vm.runInNewContext(code, {require: () => foo, exports}); + expect(foo.a).toHaveBeenCalledTimes(1); + expect(exports.a).toBe(foo.a); + }, + ); + const transform = ( source: ReadonlyArray, plugins?: Plugins = [[inlineRequiresPlugin, {}]], diff --git a/packages/metro-transform-plugins/src/inline-requires-plugin.js b/packages/metro-transform-plugins/src/inline-requires-plugin.js index d276e9199c..49229851d8 100644 --- a/packages/metro-transform-plugins/src/inline-requires-plugin.js +++ b/packages/metro-transform-plugins/src/inline-requires-plugin.js @@ -17,7 +17,6 @@ import type { MemberExpression, Node, Program, - Statement, } from '@babel/types'; type Types = Babel['types']; @@ -95,6 +94,28 @@ export default ({types: t, traverse}: Babel): PluginObj => ({ } const program: Program = programNode; + // Positions of identifiers in program order, the order in which + // `scope.crawl()` collects references. Built on first use, and rebuilt + // if a reference created since is missing. + let identifierOrder: ?Map = null; + function getIdentifierOrder( + nodes: ReadonlyArray, + ): Map { + if ( + identifierOrder == null || + nodes.some(node => !identifierOrder?.has(node)) + ) { + const order = new Map(); + path.traverse({ + Identifier(identifierPath) { + order.set(identifierPath.node, order.size); + }, + }); + identifierOrder = order; + } + return identifierOrder; + } + // Inlines `const {a, b: c} = require('m')` by replacing each reference // to `a` or `c` with `require('m').a` or `require('m').b`. When calls // are memoized, all properties share one memoized module, so each @@ -123,12 +144,24 @@ export default ({types: t, traverse}: Babel): PluginObj => ({ references.push({key, referencePath}); } } + if ( + !references.every(({referencePath}) => + canInlineAt(t, referencePath), + ) + ) { + return; + } // A memoized require is only assigned by the first reference in a - // scope, so references are replaced in source order. + // scope, so references are replaced in program order. Source + // positions can't be used for this, because nodes inserted by + // earlier plugins (e.g. `exports.a = a`) have none. + const order = getIdentifierOrder( + references.map(({referencePath}) => referencePath.node), + ); references.sort( (a, b) => - (a.referencePath.node.start ?? 0) - - (b.referencePath.node.start ?? 0), + (order.get(a.referencePath.node) ?? 0) - + (order.get(b.referencePath.node) ?? 0), ); const declarator = declarationPath.node; @@ -165,62 +198,46 @@ export default ({types: t, traverse}: Babel): PluginObj => ({ references.length > 0 && !nonMemoizedModules.has(moduleName) ) { - const name = path.scope.generateUid(moduleName); - declarationPath.remove(); - if ( - addStmtToBlock( - program, - t.variableDeclaration('var', [ - t.variableDeclarator(t.identifier(name)), - ]), - 0, - ) - ) { - memoVarName = name; - } + memoVarName = path.scope.generateUid(moduleName); + program.body.unshift( + t.variableDeclaration('var', [ + t.variableDeclarator(t.identifier(memoVarName)), + ]), + ); } - let thrown = false; const scopesWithInlinedRequire = new Set(); for (const {key, referencePath} of references) { - try { - referencePath.scope.rename(requireFnName); - if (memoVarName == null) { - referencePath.replaceWith(getMember(getRequireExpr(), key)); - continue; - } - referencePath.scope.rename(memoVarName); - const memoOrCall = t.logicalExpression( - '||', + referencePath.scope.rename(requireFnName); + if (memoVarName == null) { + referencePath.replaceWith(getMember(getRequireExpr(), key)); + continue; + } + referencePath.scope.rename(memoVarName); + const memoOrCall = t.logicalExpression( + '||', + t.identifier(memoVarName), + t.assignmentExpression( + '=', t.identifier(memoVarName), - t.assignmentExpression( - '=', - t.identifier(memoVarName), - getRequireExpr(), - ), + getRequireExpr(), + ), + ); + // Reuse the memoized module directly once this scope has + // assigned it, as for identifier aliases below. + if (!isDirectlyEnclosedByBlock(t, referencePath)) { + referencePath.replaceWith(getMember(memoOrCall, key)); + } else if (scopesWithInlinedRequire.has(referencePath.scope)) { + referencePath.replaceWith( + getMember(t.identifier(memoVarName), key), ); - // Reuse the memoized module directly once this scope has - // assigned it, as for identifier aliases above. - if (!isDirectlyEnclosedByBlock(t, referencePath)) { - referencePath.replaceWith(getMember(memoOrCall, key)); - } else if (scopesWithInlinedRequire.has(referencePath.scope)) { - referencePath.replaceWith( - getMember(t.identifier(memoVarName), key), - ); - } else { - referencePath.replaceWith(getMember(memoOrCall, key)); - scopesWithInlinedRequire.add(referencePath.scope); - } - } catch (error) { - thrown = true; + } else { + referencePath.replaceWith(getMember(memoOrCall, key)); + scopesWithInlinedRequire.add(referencePath.scope); } } - // If a replacement failed (e.g. replacing a type annotation), - // avoid removing the initial require just to be safe. - if (!thrown && declarationPath.node != null) { - declarationPath.remove(); - } + declarationPath.remove(); } path.scope.crawl(); @@ -256,6 +273,16 @@ export default ({types: t, traverse}: Babel): PluginObj => ({ ) { return; } + for (const referencePath of binding.referencePaths) { + excludeMemberAssignment(moduleName, referencePath, state); + } + if ( + !binding.referencePaths.every(referencePath => + canInlineAt(t, referencePath), + ) + ) { + return; + } const init: Expression = maybeInit; const initPath = declarationPath.get('init'); @@ -271,7 +298,6 @@ export default ({types: t, traverse}: Babel): PluginObj => ({ enter: path => deleteLocation(path.node), }); - let thrown = false; const memoVarName = parseResult.identifierName; // Whether the module has a "var foo" at program scope, used to @@ -290,9 +316,12 @@ export default ({types: t, traverse}: Babel): PluginObj => ({ const varInitStmt = t.variableDeclaration('var', [ t.variableDeclarator(t.identifier(memoVarName)), ]); - // Must remove the declaration path + // The declaration must be removed before references are + // rewritten, so that renaming any shadowing bindings to + // `memoVarName` doesn't rename the original binding. declarationPath.remove(); - hasMemoVar = addStmtToBlock(programNode, varInitStmt, 0); + program.body.unshift(varInitStmt); + hasMemoVar = true; } function getMemoOrCallExpr() { @@ -312,41 +341,34 @@ export default ({types: t, traverse}: Babel): PluginObj => ({ const scopesWithInlinedRequire = new Set(); for (const referencePath of binding.referencePaths) { - excludeMemberAssignment(moduleName, referencePath, state); - try { - referencePath.scope.rename(requireFnName); - if (hasMemoVar) { - referencePath.scope.rename(memoVarName); - // Swap the local reference with (v || v = require(m)), - // unless it is directly enclosed. - if (!isDirectlyEnclosedByBlock(t, referencePath)) { - referencePath.replaceWith(getMemoOrCallExpr()); - continue; - } - // if the current scope already has a (v || v = require(m)) - // expression for module m, use identifier reference v - // instead. Else use the full (v || v = require(m)) and - // register the current scope for subsequent references. - if (scopesWithInlinedRequire.has(referencePath.scope)) { - referencePath.replaceWith(t.identifier(memoVarName)); - } else { - referencePath.replaceWith(getMemoOrCallExpr()); - scopesWithInlinedRequire.add(referencePath.scope); - } + referencePath.scope.rename(requireFnName); + if (hasMemoVar) { + referencePath.scope.rename(memoVarName); + // Swap the local reference with (v || v = require(m)), + // unless it is directly enclosed. + if (!isDirectlyEnclosedByBlock(t, referencePath)) { + referencePath.replaceWith(getMemoOrCallExpr()); + continue; + } + // if the current scope already has a (v || v = require(m)) + // expression for module m, use identifier reference v + // instead. Else use the full (v || v = require(m)) and + // register the current scope for subsequent references. + if (scopesWithInlinedRequire.has(referencePath.scope)) { + referencePath.replaceWith(t.identifier(memoVarName)); } else { - const refExpr = t.cloneDeep(init); - // $FlowFixMe[prop-missing] - refExpr.METRO_INLINE_REQUIRES_INIT_LOC = initLoc; - referencePath.replaceWith(refExpr); + referencePath.replaceWith(getMemoOrCallExpr()); + scopesWithInlinedRequire.add(referencePath.scope); } - } catch (error) { - thrown = true; + } else { + const refExpr = t.cloneDeep(init); + // $FlowFixMe[prop-missing] + refExpr.METRO_INLINE_REQUIRES_INIT_LOC = initLoc; + referencePath.replaceWith(refExpr); } } - // If a replacement failed (e.g. replacing a type annotation), - // avoid removing the initial require just to be safe. - if (!thrown && declarationPath.node != null) { + if (declarationPath.node != null) { declarationPath.remove(); } }, @@ -679,18 +701,22 @@ function isDirectlyEnclosedByBlock(t: Types, path: NodePath) { return true; } -// insert statement to the beginning of the scope block -function addStmtToBlock(block: Program, stmt: Statement, idx: number): boolean { - const scopeBody = block.body; - if (Array.isArray(scopeBody)) { - // if the code is inside global scope - scopeBody.splice(idx, 0, stmt); +// Whether an expression such as `require('m').a` can replace the reference at +// `referencePath`. It can't in an `export {a}` specifier or a type annotation, +// for instance. This runs Babel's own check for `replaceWith`, before anything +// is rewritten. +function canInlineAt(t: Types, referencePath: NodePath<>): boolean { + if (referencePath.inList) { return true; - } else if (scopeBody && Array.isArray(scopeBody.body)) { - // if the code is inside function scope - scopeBody.body.splice(idx, 0, stmt); + } + try { + t.validate( + referencePath.parent, + String(referencePath.key), + t.memberExpression(t.identifier('m'), t.identifier('a')), + ); return true; - } else { + } catch (error) { return false; } } From a52a5097e9b7c43dfb6f3219465060f4801f8cf1 Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:06:45 +0100 Subject: [PATCH 2/3] Inline requires: Keep the require of an empty destructuring pattern --- .../src/__tests__/inline-requires-plugin-test.js | 5 +++++ .../metro-transform-plugins/src/inline-requires-plugin.js | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/metro-transform-plugins/src/__tests__/inline-requires-plugin-test.js b/packages/metro-transform-plugins/src/__tests__/inline-requires-plugin-test.js index cf1b47bf49..d7e626dd4c 100644 --- a/packages/metro-transform-plugins/src/__tests__/inline-requires-plugin-test.js +++ b/packages/metro-transform-plugins/src/__tests__/inline-requires-plugin-test.js @@ -271,6 +271,11 @@ const TEST_CASES: TestCases = { snapshot: true, }, + 'ignores empty destructured requires': { + code: ['const {} = require("foo");'].join('\n'), + snapshot: false, + }, + 'ignores destructured requires with defaults': { code: ['const {', ' a = 1', '} = require("foo");', 'a();'].join('\n'), snapshot: false, diff --git a/packages/metro-transform-plugins/src/inline-requires-plugin.js b/packages/metro-transform-plugins/src/inline-requires-plugin.js index 49229851d8..b8041c8004 100644 --- a/packages/metro-transform-plugins/src/inline-requires-plugin.js +++ b/packages/metro-transform-plugins/src/inline-requires-plugin.js @@ -559,7 +559,8 @@ type InlineableDestructuring = { }; // Matches a top-level `const {a, 'b': c} = require('m')`. Patterns with -// defaults, rest elements, nesting or computed keys are left alone. +// defaults, rest elements, nesting or computed keys are left alone, as are +// empty patterns, whose require is kept for its side effects. function parseInlineableDestructuring( path: NodePath, state: State, @@ -598,6 +599,9 @@ function parseInlineableDestructuring( } properties.push({key, localName: property.value.name}); } + if (properties.length === 0) { + return null; + } return { declarationPath, From a97bb2756d7548e6ccef76f9c2b36672c1b93c23 Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Fri, 23 Oct 2026 20:18:55 +0100 Subject: [PATCH 3/3] Inline requires: Order destructured references by tree position --- .../src/inline-requires-plugin.js | 78 ++++++++++++------- 1 file changed, 50 insertions(+), 28 deletions(-) diff --git a/packages/metro-transform-plugins/src/inline-requires-plugin.js b/packages/metro-transform-plugins/src/inline-requires-plugin.js index b8041c8004..e79fce77dc 100644 --- a/packages/metro-transform-plugins/src/inline-requires-plugin.js +++ b/packages/metro-transform-plugins/src/inline-requires-plugin.js @@ -94,28 +94,6 @@ export default ({types: t, traverse}: Babel): PluginObj => ({ } const program: Program = programNode; - // Positions of identifiers in program order, the order in which - // `scope.crawl()` collects references. Built on first use, and rebuilt - // if a reference created since is missing. - let identifierOrder: ?Map = null; - function getIdentifierOrder( - nodes: ReadonlyArray, - ): Map { - if ( - identifierOrder == null || - nodes.some(node => !identifierOrder?.has(node)) - ) { - const order = new Map(); - path.traverse({ - Identifier(identifierPath) { - order.set(identifierPath.node, order.size); - }, - }); - identifierOrder = order; - } - return identifierOrder; - } - // Inlines `const {a, b: c} = require('m')` by replacing each reference // to `a` or `c` with `require('m').a` or `require('m').b`. When calls // are memoized, all properties share one memoized module, so each @@ -155,13 +133,17 @@ export default ({types: t, traverse}: Babel): PluginObj => ({ // scope, so references are replaced in program order. Source // positions can't be used for this, because nodes inserted by // earlier plugins (e.g. `exports.a = a`) have none. - const order = getIdentifierOrder( - references.map(({referencePath}) => referencePath.node), + const positions = new Map( + references.map(({referencePath}) => [ + referencePath, + getTreePosition(t, referencePath), + ]), ); - references.sort( - (a, b) => - (order.get(a.referencePath.node) ?? 0) - - (order.get(b.referencePath.node) ?? 0), + references.sort((a, b) => + compareTreePositions( + positions.get(a.referencePath) ?? [], + positions.get(b.referencePath) ?? [], + ), ); const declarator = declarationPath.node; @@ -705,6 +687,46 @@ function isDirectlyEnclosedByBlock(t: Types, path: NodePath) { return true; } +// The position of `path` in the tree, as the index of each step's field in +// Babel's visitor keys, followed by its index in a list. Comparing positions +// orders paths as Babel traverses them, and as `scope.crawl()` collects +// references. +function getTreePosition(t: Types, path: NodePath<>): Array { + const visitorKeys: {readonly [type: string]: ReadonlyArray} = + // VISITOR_KEYS is missing from flow-typed's @babel/types. + // $FlowFixMe[prop-missing] + // $FlowFixMe[incompatible-type] + t.VISITOR_KEYS; + const position: Array = []; + for (const current of path.getAncestry()) { + if (current.parentPath == null) { + break; + } + if (current.listKey != null) { + position.push(Number(current.key)); + } + position.push( + visitorKeys[current.parent.type].indexOf( + // $FlowFixMe[incompatible-type] Only the root path has a null key. + current.listKey ?? current.key, + ), + ); + } + return position.reverse(); +} + +function compareTreePositions( + a: ReadonlyArray, + b: ReadonlyArray, +): number { + for (let i = 0; i < Math.min(a.length, b.length); i++) { + if (a[i] !== b[i]) { + return a[i] - b[i]; + } + } + return a.length - b.length; +} + // Whether an expression such as `require('m').a` can replace the reference at // `referencePath`. It can't in an `export {a}` specifier or a type annotation, // for instance. This runs Babel's own check for `replaceWith`, before anything