fix(ses) exported namespace consistency between VirtualModuleInstance and ModuleInstance#3246
fix(ses) exported namespace consistency between VirtualModuleInstance and ModuleInstance#3246naugtur wants to merge 8 commits into
Conversation
Add a test that compares Node.js native ESM and a SES Compartment when one module does `import * as foo from './a'; foo.x = 'bar'` and another module later imports `x` from the same module. Both runtimes prevent the override (assignment throws TypeError, later imports see the original value), but the protection mechanism differs in observable ways: Node.js exposes data descriptors with writable: true on a non-frozen, non-extensible namespace, while SES exposes accessor descriptors on a frozen target. The test pins down both the parity behaviors and the structural differences. These differences are known discrepancies the authors have found no way to emulate faithfully. The fixture is loaded once from disk and fed to both runtimes (Node.js via subprocess, the Compartment via @endo/module-source) so the two sides cannot drift.
… cjs-esm between endo and node
|
|
A few cjs-compat tests are failing - their fixtures are documented with comments that Node.js is not supporting finding the named export. With this change we no longer find it either. I think we should drop these tests One set of attenuator tests is failing because it's now missing the default - should probably be introduced manually. The test finding inconsistencies in live bindings between endo and node should probably be turned into a snapshot test and issues to fix compatibility if possible. Fixing one of the inconsistencies feels like it could be a side effect of this fix, but I didn't hit that target with my current attempt. Failing tests:
|
Mechanical cleanups noted in the panel review for the mirror PR; no
substance changes to naugtur's fix.
- Run prettier on packages/ses/src/module-instance.js (drops a stray
blank line) and on import-live-bindings-interop.test.js.
- Add trailing newlines to three new fixture files under
fixtures-esm-imports-cjs-define/node_modules/.
- Add `/* global process */` to namespace-mutation.test.js and
_namespace-mutation/main.js so eslint's no-undef rule passes.
- Annotate _namespace-mutation/b.js with `// @ts-nocheck` and
`eslint-disable no-import-assign` because the fixture intentionally
attempts to write to an imported namespace member to verify that the
runtime rejects the mutation.
- Hoist the first three `await readSource(...)` calls out of the
object literal in namespace-mutation.test.js to satisfy
@jessie.js/safe-await-separator.
- Cast the result of `compartment.import('./main.js')` so tsc accepts
the namespace shape (the compartment is constructed with
`__noNamespaceBox__: true`, so import resolves to the namespace
directly, not `{ namespace }`, but the type signature still reflects
the legacy boxed shape).
These changes touch only mirror-side mechanics; the test still passes
locally on Node 22 and the eslint/typecheck failures observed in CI
are eliminated. The Node 18.x test failure in
import-live-bindings-interop is independent (Node 18 does not support
`require()` of an `.mjs` file) and is also failing on upstream
endojs/endo#3246 — substance for naugtur to address upstream.
Mechanical cleanups noted in the panel review for the mirror PR; no
substance changes to naugtur's fix.
- Run prettier on packages/ses/src/module-instance.js (drops a stray
blank line) and on import-live-bindings-interop.test.js.
- Add trailing newlines to three new fixture files under
fixtures-esm-imports-cjs-define/node_modules/.
- Add `/* global process */` to namespace-mutation.test.js and
_namespace-mutation/main.js so eslint's no-undef rule passes.
- Annotate _namespace-mutation/b.js with `// @ts-nocheck` and
`eslint-disable no-import-assign` because the fixture intentionally
attempts to write to an imported namespace member to verify that the
runtime rejects the mutation.
- Hoist the first three `await readSource(...)` calls out of the
object literal in namespace-mutation.test.js to satisfy
@jessie.js/safe-await-separator.
- Cast the result of `compartment.import('./main.js')` so tsc accepts
the namespace shape (the compartment is constructed with
`__noNamespaceBox__: true`, so import resolves to the namespace
directly, not `{ namespace }`, but the type signature still reflects
the legacy boxed shape).
These changes touch only mirror-side mechanics; the test still passes
locally on Node 22 and the eslint/typecheck failures observed in CI
are eliminated. The Node 18.x test failure in
import-live-bindings-interop is independent (Node 18 does not support
`require()` of an `.mjs` file) and is also failing on upstream
endojs/endo#3246 — substance for naugtur to address upstream.
| ' ESM importing ESM/namespaceNamedLet: both LIVE', | ||
| ' ESM importing ESM/namespaceNamedVar: both LIVE', | ||
| ' ESM importing ESM/namespaceNamedConstValue: both LIVE', | ||
| '[!] ESM importing CJS (exports.*)/namedExport_namedA: node=STATIC endo=LIVE ', |
|
|
||
| export const getSummary = async () => { | ||
| // Wait for mutations (50ms) | ||
| await new Promise(resolve => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
We can almost certainly solve this coordination problem without timers. Consider injecting promise and resolver pairs into the environment instead and coordinating on those.
| '[!] CJS importing ESM (require())/namespace_namedLet: node=LIVE endo=STATIC ', | ||
| '[!] CJS importing ESM (require())/namespace_namedVar: node=LIVE endo=STATIC ', |
2 participants
TBD
Closes: #XXXX
Refs: #XXXX
Description
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Compatibility Considerations
Upgrade Considerations