Skip to content

Commit

Permalink
Distinguish source ESM imports and propagate isESMImport to Resolutio…
Browse files Browse the repository at this point in the history
…nContext (#1376)

Summary:
Correct `package.json#exports` and `package.json#imports` support requires that we assert an `"import"` or `"require"` [condition](https://nodejs.org/docs/latest-v22.x/api/packages.html#conditional-exports).

From the Node.js (where this spec originated) docs:

 ```
 "import" - matches when the package is loaded via import or import(), or via any top-level import or resolve operation by the ECMAScript module loader. Applies regardless of the module format of the target file. Always mutually exclusive with "require".
"require" - matches when the package is loaded via require(). The referenced file should be loadable with require() although the condition matches regardless of the module format of the target file. Expected formats include CommonJS, JSON, native addons, and ES modules if --experimental-require-module is enabled. Always mutually exclusive with "import".
```

Currently, we don't distinguish between import-ish and require-ish dependencies. This modifies `collectDependencies` to add `isESMImport` to the collected `DependencyData` and dependency key (so that for example `import('foo')` and `require('foo')` in the same file are considered distinct edges with potentially different resolutions), and exposes it as an (optional - for backwards compatibility) field on `ResolutionContext`.

Changelog:
```
 - **[Feature]**: Add `isESMImport` to `ResolutionContext` to distinguish resolution based on whether the dependency is a `require()` or static/async `import`.
```

Pull Request resolved: #1376

Test Plan:
New unit tests for `collectDependencies`

E2E after using this to assert conditions for `unstable_enablePackageExports`, see D70462551:
 {F1975581656}

```js1 jest xplat/js/tools/metro/packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js -t isESMImport
Determining test suites to run...
 PASS  xplat/js/tools/metro/packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js
  ○ skipped collects unique dependency identifiers and transforms the AST
  ○ skipped uses dependencyMapName parameter as-is if provided
  ○ skipped collects asynchronous dependencies
  ○ skipped collects asynchronous dependencies with keepRequireNames: false
  ○ skipped distinguishes sync and async dependencies on the same module
  ○ skipped distinguishes sync and async dependencies on the same module; reverse order
  ○ skipped exposes a string as `dependencyMapName` even without collecting dependencies
  ○ skipped ignores require functions defined defined by lower scopes
  ○ skipped collects imports
  ○ skipped collects export from
  ○ skipped records locations of dependencies
  ○ skipped integration: records locations of inlined dependencies (Metro ESM)
  ○ skipped integration: records locations of inlined dependencies (Babel ESM)
  ○ skipped uses the dependency transformer specified in the options to transform the dependency calls
  ○ skipped collects require.resolveWeak calls
  require.context
    ○ skipped does not extract/transform if feature is disabled
    ○ skipped can omit 2nd-4th arguments
    ○ skipped can pass undefined for 2nd-4th arguments
    ○ skipped can understand constant assignments
    ○ skipped can understand regex constant assignments
    ○ skipped distinguishes require from require.context
    ○ skipped distinguishes require.context based on path
    ○ skipped distinguishes require.context based on trailing slash in path
    ○ skipped distinguishes between backslash and slash in path
    ○ skipped distinguishes require.context based on `recursive`
    ○ skipped distinguishes require.context based on filter pattern
    ○ skipped distinguishes require.context based on filter flags
    ○ skipped distinguishes require.context based on mode
    ○ skipped asserts invalid first argument
    ○ skipped asserts invalid second argument
    ○ skipped asserts invalid third argument
    ○ skipped asserts invalid fourth argument
    ○ skipped asserts invalid fourth argument enum value
    ○ skipped asserts too many arguments
    ○ skipped asserts no arguments
  import() prefetching
    ○ skipped collects prefetch calls
    ○ skipped keepRequireNames: false
    ○ skipped distinguishes between import and prefetch dependncies on the same module
  require.unstable_importMaybeSync()
    ○ skipped collects require.unstable_importMaybeSync calls
    ○ skipped keepRequireNames: false
    ○ skipped distinguishes between require.unstable_importMaybeSync and prefetch dependencies on the same module
  Evaluating static arguments
    ○ skipped supports template literals as arguments
    ○ skipped supports template literals with static interpolations
    ○ skipped throws template literals with dyncamic interpolations
    ○ skipped throws on tagged template literals
    ○ skipped supports multiple static strings concatenated
    ○ skipped supports concatenating strings and template literasl
    ○ skipped supports using static variables in require statements
    ○ skipped throws when requiring non-strings
    ○ skipped throws at runtime when requiring non-strings with special option
  optional dependencies
    ○ skipped dependency in try-block within 1-level will be optional
    ○ skipped nested try-block follows the inner-most scope
    ○ skipped can handle single-line statement
    ○ skipped independent of sibling context
    ○ skipped ignores require functions defined by lower scopes
    ○ skipped supports using static variables in require statements
    ○ skipped can exclude optional dependency
    ○ skipped collapses optional and non-optional requires of the same module
    isESMImport
      ✓ distinguishes require calls, static imports and async imports (17 ms)

Test Suites: 1 passed, 1 total
Tests:       58 skipped, 1 passed, 59 total
Snapshots:   0 total
Time:        0.501 s, estimated 1 s
Ran all test suites matching /xplat\/js\/tools\/metro\/packages\/metro\/src\/ModuleGraph\/worker\/__tests__\/collectDependencies-test.js/i with tests matching "isESMImport".
```

Reviewed By: huntie

Differential Revision: D64981658

Pulled By: robhogan

fbshipit-source-id: 6ac3fa84a65fa126ce8dba85ce2093d103706ad0
  • Loading branch information
robhogan authored and facebook-github-bot committed Mar 2, 2025
1 parent f0d4a00 commit d187fb2
Show file tree
Hide file tree
Showing 26 changed files with 457 additions and 27 deletions.
6 changes: 6 additions & 0 deletions docs/Resolution.md
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,12 @@ Given the path to a `package.json` file, returns the parsed file contents.

Given a candidate absolute module path that may exist under a package, locates and returns the closest package root (working upwards from the given path, stopping at the nearest `node_modules`), parsed `package.json` contents, and the package-relative path of the given path.

#### `isESMImport?: boolean`

Whether the dependency to be resolved was declared with an ESM import, ("import x from 'y'" or "await import('z')"), or a CommonJS "require". Corresponds to the criteria Node.js uses to assert an "import" resolution condition, vs "require".

Always equal to dependency.data.isESMImport where dependency is provided, but may be used for resolution.

#### `resolveHasteModule: string => ?string`

Resolves a Haste module name to an absolute path. Returns `null` if no such module exists.
Expand Down
3 changes: 3 additions & 0 deletions packages/metro-babel-transformer/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,13 @@ export type BabelFileFunctionMapMetadata = $ReadOnly<{
mappings: string,
}>;

export type BabelFileImportLocsMetadata = $ReadOnlySet<string>;

export type MetroBabelFileMetadata = {
...BabelFileMetadata,
metro?: ?{
functionMap?: ?BabelFileFunctionMapMetadata,
unstable_importDeclarationLocs?: ?BabelFileImportLocsMetadata,
...
},
...
Expand Down
1 change: 1 addition & 0 deletions packages/metro-resolver/src/__tests__/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export function createResolutionContext(
realPath: candidate.realPath,
};
},
isESMImport: false,
mainFields: ['browser', 'main'],
nodeModulesPaths: [],
preferNativePlatform: false,
Expand Down
11 changes: 11 additions & 0 deletions packages/metro-resolver/src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,17 @@ export type ResolutionContext = $ReadOnly<{
*/
dependency?: TransformResultDependency,

/**
* Whether the dependency to be resolved was declared with an ESM import,
* ("import x from 'y'" or "await import('z')"), or a CommonJS "require".
* Corresponds to the criteria Node.js uses to assert an "import"
* resolution condition, vs "require".
*
* Always equal to dependency.data.isESMImport where dependency is provided,
* but may be used for resolution.
*/
isESMImport?: boolean,

/**
* Synchonously returns information about a given absolute path, including
* whether it exists, whether it is a file or directory, and its absolute
Expand Down
11 changes: 11 additions & 0 deletions packages/metro-resolver/types/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,17 @@ export interface ResolutionContext {
*/
readonly dependency?: TransformResultDependency;

/**
* Whether the dependency to be resolved was declared with an ESM import,
* ("import x from 'y'" or "await import('z')"), or a CommonJS "require".
* Corresponds to the criteria Node.js uses to assert an "import"
* resolution condition, vs "require".
*
* Always equal to dependency.data.isESMImport where dependency is provided,
* but may be used for resolution.
*/
readonly isESMImport?: boolean;

/**
* Synchonously returns information about a given absolute path, including
* whether it exists, whether it is a file or directory, and its absolute
Expand Down
21 changes: 19 additions & 2 deletions packages/metro-transform-worker/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ const {
InvalidRequireCallError: InternalInvalidRequireCallError,
} = require('metro/src/ModuleGraph/worker/collectDependencies');
const generateImportNames = require('metro/src/ModuleGraph/worker/generateImportNames');
const {
importLocationsPlugin,
locToKey,
} = require('metro/src/ModuleGraph/worker/importLocationsPlugin');
const JsFileWrapping = require('metro/src/ModuleGraph/worker/JsFileWrapping');
const nullthrows = require('nullthrows');

Expand Down Expand Up @@ -148,6 +152,7 @@ type JSFile = $ReadOnly<{
ast?: ?BabelNodeFile,
type: JSFileType,
functionMap: FBSourceFunctionMap | null,
unstable_importDeclarationLocs?: ?$ReadOnlySet<string>,
}>;

type JSONFile = {
Expand Down Expand Up @@ -381,6 +386,7 @@ async function transformJS(
wrappedAst = JsFileWrapping.wrapPolyfill(ast);
} else {
try {
const importDeclarationLocs = file.unstable_importDeclarationLocs ?? null;
const opts = {
asyncRequireModulePath: config.asyncRequireModulePath,
dependencyTransformer:
Expand All @@ -396,6 +402,11 @@ async function transformJS(
allowOptionalDependencies: config.allowOptionalDependencies,
dependencyMapName: config.unstable_dependencyMapReservedName,
unstable_allowRequireContext: config.unstable_allowRequireContext,
unstable_isESMImportAtSource:
importDeclarationLocs != null
? (loc: BabelSourceLocation) =>
importDeclarationLocs.has(locToKey(loc))
: null,
};
({ast, dependencies, dependencyMapName} = collectDependencies(ast, opts));
} catch (error) {
Expand Down Expand Up @@ -531,8 +542,12 @@ async function transformJSWithBabel(
const transformer: BabelTransformer = require(babelTransformerPath);

const transformResult = await transformer.transform(
// functionMapBabelPlugin populates metadata.metro.functionMap
getBabelTransformArgs(file, context, [functionMapBabelPlugin]),
getBabelTransformArgs(file, context, [
// functionMapBabelPlugin populates metadata.metro.functionMap
functionMapBabelPlugin,
// importLocationsPlugin populates metadata.metro.unstable_importDeclarationLocs
importLocationsPlugin,
]),
);

const jsFile: JSFile = {
Expand All @@ -543,6 +558,8 @@ async function transformJSWithBabel(
// Fallback to deprecated explicitly-generated `functionMap`
transformResult.functionMap ??
null,
unstable_importDeclarationLocs:
transformResult.metadata?.metro?.unstable_importDeclarationLocs,
};

return await transformJS(jsFile, context);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ const fooModule: Module<> = {
'./bar',
{
absolutePath: '/root/bar',
data: {data: {asyncType: null, locs: [], key: './bar'}, name: './bar'},
data: {
data: {asyncType: null, isESMImport: false, locs: [], key: './bar'},
name: './bar',
},
},
],
]),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ function createModule(
dep,
{
absolutePath: `/root/${dep}.js`,
data: {data: {asyncType: null, locs: [], key: dep}, name: dep},
data: {
data: {asyncType: null, isESMImport: false, locs: [], key: dep},
name: dep,
},
},
]),
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ const fooModule: Module<> = {
'./bar',
{
absolutePath: '/root/bar.js',
data: {data: {asyncType: null, locs: [], key: './bar'}, name: './bar'},
data: {
data: {asyncType: null, isESMImport: false, locs: [], key: './bar'},
name: './bar',
},
},
],
]),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,20 @@ beforeEach(() => {
'bar',
{
absolutePath: '/bar.js',
data: {data: {asyncType: null, locs: [], key: 'bar'}, name: 'bar'},
data: {
data: {asyncType: null, isESMImport: false, locs: [], key: 'bar'},
name: 'bar',
},
},
],
[
'baz',
{
absolutePath: '/baz.js',
data: {data: {asyncType: null, locs: [], key: 'baz'}, name: 'baz'},
data: {
data: {asyncType: null, isESMImport: false, locs: [], key: 'baz'},
name: 'baz',
},
},
],
]),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,12 @@ describe('DeltaCalculator + require.context', () => {
absolutePath: '/ctx?ctx=xxx',
data: {
name: 'ctx',
data: {key: 'ctx?ctx=xxx', asyncType: null, locs: []},
data: {
key: 'ctx?ctx=xxx',
asyncType: null,
isESMImport: false,
locs: [],
},
},
},
],
Expand All @@ -109,7 +114,12 @@ describe('DeltaCalculator + require.context', () => {
absolutePath: '/ctx/foo',
data: {
name: 'foo',
data: {key: 'foo', asyncType: null, locs: []},
data: {
key: 'foo',
asyncType: null,
isESMImport: false,
locs: [],
},
},
},
],
Expand Down
28 changes: 24 additions & 4 deletions packages/metro/src/DeltaBundler/__tests__/DeltaCalculator-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,12 @@ describe.each(['linux', 'win32'])('DeltaCalculator (%s)', osPlatform => {
absolutePath: p('/foo'),
data: {
name: 'foo',
data: {key: 'foo', asyncType: null, locs: []},
data: {
key: 'foo',
asyncType: null,
isESMImport: false,
locs: [],
},
},
},
],
Expand All @@ -104,7 +109,12 @@ describe.each(['linux', 'win32'])('DeltaCalculator (%s)', osPlatform => {
absolutePath: p('/bar'),
data: {
name: 'bar',
data: {key: 'bar', asyncType: null, locs: []},
data: {
key: 'bar',
asyncType: null,
isESMImport: false,
locs: [],
},
},
},
],
Expand All @@ -114,7 +124,12 @@ describe.each(['linux', 'win32'])('DeltaCalculator (%s)', osPlatform => {
absolutePath: p('/baz'),
data: {
name: 'baz',
data: {key: 'baz', asyncType: null, locs: []},
data: {
key: 'baz',
asyncType: null,
isESMImport: false,
locs: [],
},
},
},
],
Expand All @@ -132,7 +147,12 @@ describe.each(['linux', 'win32'])('DeltaCalculator (%s)', osPlatform => {
absolutePath: p('/qux'),
data: {
name: 'qux',
data: {key: 'qux', asyncType: null, locs: []},
data: {
key: 'qux',
asyncType: null,
isESMImport: false,
locs: [],
},
},
},
],
Expand Down
2 changes: 2 additions & 0 deletions packages/metro/src/DeltaBundler/__tests__/Graph-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ beforeEach(async () => {
name: dep.name,
data: {
asyncType: null,
isESMImport: false,
// $FlowFixMe[missing-empty-array-annot]
locs: [],
// $FlowFixMe[incompatible-call]
Expand Down Expand Up @@ -3498,6 +3499,7 @@ describe('reorderGraph', () => {
data: {
data: {
asyncType: null,
isESMImport: false,
locs: [],
key: path.substr(1),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ TestGraph {
"data": Object {
"data": Object {
"asyncType": null,
"isESMImport": false,
"key": "LB7P4TKrvfdUdViBXGaVopqz7Os=",
"locs": Array [],
},
Expand Down Expand Up @@ -39,6 +40,7 @@ TestGraph {
"data": Object {
"data": Object {
"asyncType": null,
"isESMImport": false,
"key": "W+de6an7x9bzpev84O0W/hS4K8U=",
"locs": Array [],
},
Expand All @@ -50,6 +52,7 @@ TestGraph {
"data": Object {
"data": Object {
"asyncType": null,
"isESMImport": false,
"key": "x6e9Oz1JO0QPfIBBjUad2qqGFjI=",
"locs": Array [],
},
Expand Down Expand Up @@ -137,6 +140,7 @@ TestGraph {
"data": Object {
"data": Object {
"asyncType": null,
"isESMImport": false,
"key": "LB7P4TKrvfdUdViBXGaVopqz7Os=",
"locs": Array [],
},
Expand Down
11 changes: 9 additions & 2 deletions packages/metro/src/DeltaBundler/__tests__/buildSubgraph-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,17 @@ import nullthrows from 'nullthrows';
const makeTransformDep = (
name: string,
asyncType: null | 'weak' | 'async' = null,
isESMImport: boolean = false,
contextParams?: RequireContextParams,
): TransformResultDependency => ({
name,
data: {key: 'key-' + name, asyncType, locs: [], contextParams},
data: {
key: 'key-' + name + (isESMImport ? '-import' : ''),
asyncType,
isESMImport,
locs: [],
contextParams,
},
});

class BadTransformError extends Error {}
Expand All @@ -40,7 +47,7 @@ describe('GraphTraversal', () => {
[
'/entryWithContext',
[
makeTransformDep('virtual', null, {
makeTransformDep('virtual', null, false, {
filter: {
pattern: 'contextMatch.*',
flags: 'i',
Expand Down
1 change: 1 addition & 0 deletions packages/metro/src/DeltaBundler/__tests__/resolver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ function dep(name: string): TransformResultDependency {
name,
data: {
asyncType: null,
isESMImport: false,
key: name,
locs: [],
},
Expand Down
5 changes: 5 additions & 0 deletions packages/metro/src/DeltaBundler/types.flow.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ export type TransformResultDependency = $ReadOnly<{
* If not null, this dependency is due to a dynamic `import()` or `__prefetchImport()` call.
*/
asyncType: AsyncDependencyType | null,
/**
* True if the dependency is declared with a static "import x from 'y'" or
* an import() call.
*/
isESMImport: boolean,
/**
* The dependency is enclosed in a try/catch block.
*/
Expand Down
1 change: 1 addition & 0 deletions packages/metro/src/HmrServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ class HmrServer<TClient: Client> {
data: {
key: entryFile,
asyncType: null,
isESMImport: false,
locs: [],
},
},
Expand Down
Loading

0 comments on commit d187fb2

Please sign in to comment.