Skip to content

Commit

Permalink
Include named exports in CJS shims when using importSync
Browse files Browse the repository at this point in the history
`importSync()` is intended to work like `import()` (buy sync),
which, in turns, is expected to work like a namespace import –
`import * as NS from "foo";`. In general, we would expect that to
be interchangable with `const NS = importSync("foo");`.

Prior to this commit, this is not true for commonjs packages:

```js
import * as QUnit1 from "qunit";
// => { QUnit, assert, begin, config, ... }

QUnit1.assert // => function

const QUnit2 = importSync("qunit");
// => { default: { QUnit, assert, begin, config, ... } }

QUnit2.assert // => undefined
```

Of course, using ES imports on commonjs packages is, in itself, an
ill-defined concept, but nevertheless, it is a very common interop
requirement.

The previous behavior is introduced in embroider-build#1076. The intent appears to
be fixing a different interop issue, where we would expect these to
work the same way:

```js
import QUnit1 from "qunit";
// => { Qunit, assert, begin, config, ... }

QUnit1.assert // => function

const QUnit2 = importSync("qunit").default;
// => { QUnit, assert, begin, config, ... }

QUnit2.assert // => function
```

The fix in embroider-build#1076, however, broke the exepctation that `import()`
should behave like a namespace improt statement for CJS modules.

This commit attempts to satisfy both of these requirements, with
one caveat: `importSync("foo").default` is present on the shimed
CJS modules, but in `import * as FOO from "foo";`, `FOO.default`
is undefined.

Arguably, this is a bug in webpack's implementation – if you are
able to write an import statement that references the default
export, you should be able to do the same in the namespace import
as per the spec.

So, the implementation here is different but more correct.

Fixes embroider-build#1530
  • Loading branch information
chancancode committed Jul 13, 2023
1 parent 4f3826d commit ff85425
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 1 deletion.
1 change: 1 addition & 0 deletions packages/macros/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@
/tests/**/*.map
!/src/vendor/*.js
!/src/addon/*.js
!/src/addon/es-compat.d.ts
1 change: 1 addition & 0 deletions packages/macros/src/addon/es-compat.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default function esc(m: any): any;
2 changes: 1 addition & 1 deletion packages/macros/src/addon/es-compat.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export default function esCompat(m) {
return m?.__esModule ? m : { default: m };
return m?.__esModule ? m : { default: m, ...m };
}
27 changes: 27 additions & 0 deletions packages/macros/tests/runtime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
moduleExists,
} from '../src/index';

import esc from '../src/addon/es-compat';

const ERROR_REGEX =
/this method is really implemented at compile time via a babel plugin. If you're seeing this exception, something went wrong/;

Expand Down Expand Up @@ -53,3 +55,28 @@ describe(`type-only exports`, function () {
expect(moduleExists).toThrow(ERROR_REGEX);
});
});

describe(`es-compat`, function () {
test('ES module are untouched', function () {
let esm = {
__esModule: true,
default: class ESM {},
named: function named() {},
};

expect(esc(esm)).toEqual(esm);
});

test('CJS module are shimmed', function () {
let cjs = {
named: function named() {},
another: function another() {},
};

expect(esc(cjs).default.named).toEqual(cjs.named);
expect(esc(cjs).default.another).toEqual(cjs.another);

expect(esc(cjs).named).toEqual(cjs.named);
expect(esc(cjs).another).toEqual(cjs.another);
});
});

0 comments on commit ff85425

Please sign in to comment.