fix(compartment-mapper): Support ESM imports defined property from CJ… (

#2330)

…S from set property of CJS

Refs: Agoric/agoric-sdk#9408

## Description

If a modern JavaScript module imports a named binding from a CommonJS
module, we can hardly guarantee that it will always work. Outrageous
heroics make it work under most circumstances using a heuristic lexical
analysis of the CommonJS module to determine which names should exist on
the module’s internal namespace. Then, the CommonJS runtime must account
for all variety of shenanigans that might cause the binding for that
name to change. These include assignment to property names on `exports`,
reassignment to `module.exports`, and `defineProperty` on the _original_
`exports` object. These changes must be reflected both on the module
namespace object and on the `default` property thereof in _in every case
where this is in fact possible_.

Consider a CJS module (2.cjs) that exports a name:

```js
exports.meaning = 42;
```

Then, consider a TypeScript module (1.ts) that reexports this name:

```ts
export { meaning } from './2.cjs';
```

Which gets compiled down to a CommonJS module (1.cjs):

```js
const universe = require('./1.cjs');
Object.defineProperty(exports, 'meaning', {
  enumerable: true,
  get() {
    return universe.meaning
  }
});
```

Which gets imported by an ESM by name (0.mjs):

```js
import { meaning } from './1.cjs';
```

This has no right to work. We can trivially copy the getter from 1.cjs
to the `default` export but the internal namespace object we use to
emulate ESM in SES emits live binding notifications for every named
property and doesn’t suffer `defineProperty` well. So, in the CommonJS
emulation, we must attempt to propagate the current value from the
getter to the module environment record after the module initializes.

### Security Considerations

None.

### Scaling Considerations

None.

### Documentation Considerations

None. Folks already assume this works. It works in Node.js. Code exists
that relies on it.

### Testing Considerations

We have an existing test that exercises the case that the getter under a
defineProperty can’t be expected to succeed on the stack of
defineProperty.

### Compatibility Considerations

This increases ecosystem compatibility.

### Upgrade Considerations

None.

### PS

It didn’t have to be like this.

packages/compartment-mapper/src/parse-cjs-shared-export-wrapper.js

@@ -103,6 +103,8 @@ export const wrap = ({
     }
   };
 
+  const redefined = new Set();
+
   const originalExports = new Proxy(moduleEnvironmentRecord.default, {
     set(_target, prop, value) {
       promoteToNamedExport(prop, value);
@@ -121,6 +123,9 @@ export const wrap = ({
       // descriptor.get in the trap will cause an error.
       // Object.defineProperty is used instead of Reflect.defineProperty for better error messages.
       defineProperty(target, prop, descriptor);
+      // Make a note to propagate the value from the getter to the module
+      // environment record when the module finishes executing.
+      redefined.add(prop);
       return true;
     },
   });
@@ -176,8 +181,13 @@ export const wrap = ({
     if (exportsHaveBeenOverwritten) {
       moduleEnvironmentRecord.default = finalExports;
       for (const prop of keys(moduleEnvironmentRecord.default || {})) {
-        if (prop !== 'default')
+        if (prop !== 'default') {
           moduleEnvironmentRecord[prop] = moduleEnvironmentRecord.default[prop];
+        }
+      }
+    } else {
+      for (const prop of redefined) {
+        moduleEnvironmentRecord[prop] = moduleEnvironmentRecord.default[prop];
       }
     }
   };

packages/compartment-mapper/test/esm-imports-cjs-define.test.js

@@ -0,0 +1,23 @@
+/* eslint-disable no-underscore-dangle */
+// import "./ses-lockdown.js";
+import 'ses';
+import test from 'ava';
+
+import { scaffold } from './scaffold.js';
+
+const fixture = new URL(
+  'fixtures-esm-imports-cjs-define/0.mjs',
+  import.meta.url,
+).toString();
+
+const assertFixture = t => t.pass();
+
+const fixtureAssertionCount = 1;
+
+scaffold(
+  'fixtures-esm-imports-cjs-define',
+  test,
+  fixture,
+  assertFixture,
+  fixtureAssertionCount,
+);

packages/compartment-mapper/test/fixtures-esm-imports-cjs-define/0.mjs

@@ -0,0 +1,4 @@
+import { meaning } from './1.cjs';
+if (meaning !== 42) {
+  throw new Error('Fail');
+}

packages/compartment-mapper/test/fixtures-esm-imports-cjs-define/1.cjs

@@ -0,0 +1,8 @@
+const universe = require('./2.cjs');
+Object.defineProperty(exports, '__esModule', { value: true });
+Object.defineProperty(exports, 'meaning', {
+  enumerable: true,
+  get() {
+    return universe.meaning;
+  },
+});

packages/compartment-mapper/test/fixtures-esm-imports-cjs-define/2.cjs

@@ -0,0 +1,2 @@
+Object.defineProperty(exports, '__esModule', { value: true });
+exports.meaning = 42;

packages/compartment-mapper/test/fixtures-esm-imports-cjs-define/package.json

@@ -0,0 +1,9 @@
+{
+  "name": "app",
+  "version": "1.0.0",
+  "main": "./0.mjs",
+  "type": "commonjs",
+  "scripts": {
+    "preinstall": "echo DO NOT INSTALL TEST FIXTURES; exit -1"
+  }
+}