Skip to content

Commit 4ebd33b

Browse files
robhoganfacebook-github-bot
authored andcommitted
Support circular dependencies with experimentalImportSupport (#1429)
Summary: NOTE: This is an opt-in extension to `experimentalImportSupport`, this diff is a no-op by default. Currently, `experimentalImportSupport` transforms named imports such that they're accessed immediately at the top level, eg: ```js import {foo} from 'bar'; export function getFoo() { return foo; } ``` Becomes ```js Object.defineProperty(exports, '__esModule', { value: true }); var foo = require('bar').foo; function getFoo() { return foo; } exports.getFoo = getFoo; ``` This immediate, top-level assignment of `require('bar').foo` to `foo` problematic for two reasons: 1. In the case of circular dependencies, the module at `'bar'` may not have been fully initialised, so that `foo` might be undefined at this point. 2. In the case where `bar` defines `export let foo = 'something mutable'`, a reassignment of `foo` within `'bar'` at runtime will not be reflected by the importing module. This aims to fix 1 and get closer to a fix for 2. The new output would be: ```js Object.defineProperty(exports, '__esModule', { value: true }); var _bar = require('bar'); function getFoo() { return _bar.foo; } exports.getFoo = getFoo; ``` By lazily accessing values Differential Revision: D68394514
1 parent d8bf840 commit 4ebd33b

File tree

15 files changed

+474
-14
lines changed

15 files changed

+474
-14
lines changed

packages/metro-babel-transformer/src/index.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ type BabelTransformerOptions = $ReadOnly<{
3030
enableBabelRCLookup?: boolean,
3131
enableBabelRuntime: boolean | string,
3232
extendsBabelConfigPath?: string,
33-
experimentalImportSupport?: boolean,
33+
experimentalImportSupport?: boolean | $ReadOnly<{importAsObjects?: boolean}>,
3434
hermesParser?: boolean,
3535
hot: boolean,
3636
minify: boolean,

packages/metro-babel-transformer/types/index.d.ts

+3-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ export interface BabelTransformerOptions {
2020
readonly enableBabelRCLookup?: boolean;
2121
readonly enableBabelRuntime: boolean | string;
2222
readonly extendsBabelConfigPath?: string;
23-
readonly experimentalImportSupport?: boolean;
23+
readonly experimentalImportSupport?:
24+
| boolean
25+
| Readonly<{importAsObjects?: boolean}>;
2426
readonly hermesParser?: boolean;
2527
readonly hot: boolean;
2628
readonly minify: boolean;

packages/metro-config/src/configTypes.flow.js

+5-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,11 @@ export type ExtraTransformOptions = $ReadOnly<{
3131
preloadedModules?: $ReadOnly<{[path: string]: true, ...}> | false,
3232
ramGroups?: $ReadOnlyArray<string>,
3333
transform?: $ReadOnly<{
34-
experimentalImportSupport?: boolean,
34+
experimentalImportSupport?:
35+
| boolean
36+
| $ReadOnly<{
37+
importAsObjects?: boolean,
38+
}>,
3539
inlineRequires?:
3640
| $ReadOnly<{blockList: $ReadOnly<{[string]: true, ...}>, ...}>
3741
| boolean,

packages/metro-config/types/configTypes.d.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ export interface ExtraTransformOptions {
2626
readonly preloadedModules: Readonly<{[path: string]: true}> | false;
2727
readonly ramGroups: ReadonlyArray<string>;
2828
readonly transform: Readonly<{
29-
experimentalImportSupport: boolean;
29+
experimentalImportSupport: boolean | Readonly<{importAsObjects?: boolean}>;
3030
inlineRequires:
3131
| Readonly<{blockList: Readonly<{[path: string]: true}>}>
3232
| boolean;

packages/metro-transform-plugins/src/__tests__/import-export-plugin-test.js

+138-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
'use strict';
1313

14+
import type {Options} from '../import-export-plugin';
1415
import type {Dependency} from 'metro/src/ModuleGraph/worker/collectDependencies';
1516

1617
const {compare, transformToAst} = require('../__mocks__/test-helpers');
@@ -19,9 +20,10 @@ const importExportPlugin = require('../import-export-plugin');
1920
const {codeFrameColumns} = require('@babel/code-frame');
2021
const collectDependencies = require('metro/src/ModuleGraph/worker/collectDependencies');
2122

22-
const opts = {
23+
const opts: Options = {
2324
importAll: '_$$_IMPORT_ALL',
2425
importDefault: '_$$_IMPORT_DEFAULT',
26+
resolve: false,
2527
};
2628

2729
test('correctly transforms and extracts "import" statements', () => {
@@ -375,6 +377,141 @@ test('supports `import {default as LocalName}`', () => {
375377
`);
376378
});
377379

380+
describe('importAsObjects == true', () => {
381+
const innerOpts: Options = {...opts, importAsObjects: true};
382+
test('named import refs transform to object property access', () => {
383+
const code = `
384+
import {'foo-bar' as fooBar, baz} from 'qux';
385+
const obj = {fooBar, baz};
386+
console.log(fooBar, baz);
387+
`;
388+
const expected = `
389+
var _qux = require('qux');
390+
const obj = {
391+
fooBar: _qux["foo-bar"],
392+
baz: _qux.baz
393+
};
394+
console.log(_qux["foo-bar"], _qux.baz);
395+
`;
396+
compare([importExportPlugin], code, expected, innerOpts);
397+
});
398+
399+
test('re-exporting named imports', () => {
400+
const code = `
401+
import {'foo-bar' as fooBar, baz} from 'qux';
402+
export {fooBar, baz as myBaz};
403+
`;
404+
const expected = `
405+
Object.defineProperty(exports, '__esModule', {
406+
value: true
407+
});
408+
var _qux = require('qux');
409+
exports.fooBar = _qux["foo-bar"];
410+
exports.myBaz = _qux.baz;
411+
`;
412+
compare([importExportPlugin], code, expected, innerOpts);
413+
});
414+
415+
test('namespace import refs maintain property access', () => {
416+
const code = `
417+
import * as Foo from 'foo';
418+
console.log(Foo.bar, Foo.baz);
419+
`;
420+
const expected = `
421+
var Foo = _$$_IMPORT_ALL('foo');
422+
console.log(Foo.bar, Foo.baz);
423+
`;
424+
compare([importExportPlugin], code, expected, innerOpts);
425+
});
426+
427+
test('mixed default, default-by-name and named imports`', () => {
428+
const code = `
429+
import RN, {
430+
View,
431+
Platform as RNPlatform,
432+
default as ReactNative
433+
} from 'react-native';
434+
import ReactNative2 from 'react-native';
435+
436+
console.log(View, RNPlatform, ReactNative, RN);
437+
438+
function myFunc() {
439+
const RNPlatform = 'foo';
440+
RN.bar();
441+
React.foo();
442+
return RNPlatform;
443+
}
444+
445+
function myOtherFunc() {
446+
return RNPlatform;
447+
}
448+
`;
449+
450+
const expected = `
451+
var _reactNative = require('react-native');
452+
var RN = _$$_IMPORT_DEFAULT('react-native');
453+
var ReactNative = RN;
454+
var ReactNative2 = _$$_IMPORT_DEFAULT('react-native');
455+
console.log(_reactNative.View, _reactNative.Platform, ReactNative, RN);
456+
function myFunc() {
457+
const RNPlatform = 'foo';
458+
RN.bar();
459+
React.foo();
460+
return RNPlatform;
461+
}
462+
function myOtherFunc() {
463+
return _reactNative.Platform;
464+
}
465+
`;
466+
467+
compare([importExportPlugin], code, expected, innerOpts);
468+
469+
// Expect three dependencies mapping to the first import declaration...
470+
// 1. RN (import default)
471+
// 2. View and Platform (import named)
472+
// 3. ReactNative (import default as)
473+
// ...and one mapping to the second import declaration
474+
// 4. ReactNative2 (import default)
475+
// All should share the same dependency key (at 0) because all are resolved
476+
// identically.
477+
expect(showTransformedDeps(code)).toMatchInlineSnapshot(`
478+
"
479+
> 2 | import RN, {
480+
| ^^^^^^^^^^^^
481+
> 3 | View,
482+
| ^^^^^^^^^^^
483+
> 4 | Platform as RNPlatform,
484+
| ^^^^^^^^^^^
485+
> 5 | default as ReactNative
486+
| ^^^^^^^^^^^
487+
> 6 | } from 'react-native';
488+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ dep #0 (react-native)
489+
> 2 | import RN, {
490+
| ^^^^^^^^^^^^
491+
> 3 | View,
492+
| ^^^^^^^^^^^
493+
> 4 | Platform as RNPlatform,
494+
| ^^^^^^^^^^^
495+
> 5 | default as ReactNative
496+
| ^^^^^^^^^^^
497+
> 6 | } from 'react-native';
498+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ dep #0 (react-native)
499+
> 2 | import RN, {
500+
| ^^^^^^^^^^^^
501+
> 3 | View,
502+
| ^^^^^^^^^^^
503+
> 4 | Platform as RNPlatform,
504+
| ^^^^^^^^^^^
505+
> 5 | default as ReactNative
506+
| ^^^^^^^^^^^
507+
> 6 | } from 'react-native';
508+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ dep #0 (react-native)
509+
> 7 | import ReactNative2 from 'react-native';
510+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ dep #0 (react-native)"
511+
`);
512+
});
513+
});
514+
378515
function showTransformedDeps(code: string) {
379516
const {dependencies} = collectDependencies(
380517
transformToAst([importExportPlugin], code, opts),

0 commit comments

Comments
 (0)