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

[kriskowal]

…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:

exports.meaning = 42;

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

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

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

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):

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.

[naugtur]

I think we should add this case to the matrix in https://github.com/endojs/endo-e2e-tests (along with some new runtimes BTW) and see how it compares.

Also, I wonder if there's a noticeable change in the e2e test results form this. I remember at the time the cjs implementation was passing e2e with few exports missing and some exports being redundant.

One thing we're guaranteed to never fix is redundant default. I hope that never becomes an issue.

Anyway, I think a run of e2e on main vs this could be informative.

[edit]

updated the matrix endojs/endo-e2e-tests#10

https://github.com/endojs/endo-e2e-tests/blob/ccb193d8bdd04d03a93507090d932aced0019b27/matrix/table.md

[boneskull]

(suggestion) have a

export const DEFAULT = 'default';

somewhere and use it

[boneskull]

this means "if this fixture does not error, we're good"?

[kriskowal]

@naugtur Can I beg a ruling on this change?

[naugtur]

there could be an else before this because redefined doesn't make a lot of sense when the default export was redefined. but it should also be an empty set in that case.