feat(compartment-mapper): support treating top-level return in CJS as a way to export from module

[naugtur]

Description

One of the rarely used ways to export from a CJS module is to return a reference instead of setting module.exports to that reference. This PR adds support for that behavior and a tiny fixture in tests to ensure it works in all scenarios.

Security Considerations

N/A

Scaling Considerations

N/A

Documentation Considerations

It's part of CJS and also something very few people know. Documenting it would lead to more confusion than not.

Testing Considerations

Covered with a case in existing test coverage.

Compatibility Considerations

Increases ecosystem compatibility by the few unfortunate packages that use this way of exporting.

Upgrade Considerations

None

[Copilot]

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[Copilot]

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

packages/compartment-mapper/src/parse-pre-cjs.js:51

• Ensure that the test suite includes scenarios where functor.call returns undefined, so that the fallback to module.exports is properly validated.

const returnValue = functor.call(

packages/compartment-mapper/src/parse-cjs.js:58

• Verify that tests cover cases when functor.call returns a defined value versus undefined, ensuring consistency in export behavior.

const returnValue = functor.call(

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

• Confirm that tests validate both branches of the conditional that selects returnValue or module.exports, ensuring that shared export logic behaves as expected.

const afterExecute = returnValue => {

[boneskull]

Wait. Is this right?

Executing node -e console.log(require("/path/to/exports-return.js")) reports {} regardless of what was returned. Change it to anything else; it's always {}.

It appears that we need to assert exports-return.js returns an empty object. So... not {magicNumber: 42} at all.

It looks as if the originalExports value comes from the AST? But return must be in module scope, and module.exports needn't be (afaik). So it would be challenging to determine whether or not a module "aborts" with a return before module.exports is assigned to—and not possible in the worst case.

Static analysis can only get us as far as "there is no return in module scope, and the module.exports assignment is not in a block". That's the only case in which we can be reasonably sure about its properties. Otherwise, we won't be able to statically determine a) whether or not the module bailed before assigning exports (empty object) and/or b) which property keys are exported:

if (stuff) {
  return;
} else if (otherStuff) {
  module.exports = {foo: 'bar'};
} else {
  module.exports = {bar: 'baz'};
}

Right???

Maybe what needs to happen is:

1. Ignore whatever originalExports is, assuming we don't need it for something else
2. Ignore the return value of functor
3. Remove the conditional exportsHaveBeenOverwritten and just assume things need to be updated
4. Cross fingers that this won't break shit

cc @kriskowal who will likely find this abhorrent

[boneskull]

Might need to tweak the parser to flag a module's exports as "lol idk"

[naugtur]

I implemented this because bundlers had it claiming it to be node.js compatibility AFAIR. Now I see Node doesn't have that behavior and am confused. Will report back

[boneskull]

I think this is busted 😄

[boneskull]

We will also need to enable the allowReturnOutsideFunction option for calls to @babel/parser (if sourceType is script). This change would need to happen both in @endo/module-source and @endo/evasive-transform.

[naugtur]

This turns out to be a behavior specific to some bundlers at most, definitely not what I can observe in node.js. closing.