Include named exports in CJS shims when using importSync

[chancancode]

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:

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 #1076. The intent appears to be fixing a different interop issue, where we would expect these to work the same way:

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 #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.

[chancancode]

Didn't find an integration/end-to-end test for importing cjs packages, so settled with a unit test. If I should add a test somewhere else instead let me know.

[ef4]

Thanks. The test wasn't really doing what you wanted it to do -- that particular file is misleading and kind of silly. It was only testing build-time code and couldn't test runtime code.

I moved it into a place that will do an end-to-end test.

(We definitely need some better contributing docs on how to organize tests.)

[ef4]

Making this do the right thing on classic builds is more annoying than it would seem, given that classic builds mash all the copies of all the different versions of @embroider/macros together. It necessitates picking a new name for the module.

Then that introduced test failures under embroider, due to an outstanding issue that I'm already working on. I can land this after landing that fix.