Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(module-source): Add XS specific variant #2470

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

kriskowal
Copy link
Member

@kriskowal kriskowal commented Sep 26, 2024

Closes: #2250
Refs: #2251

Description

Toward using native XS compartments and module sources, this change adds an XS-specific variant of @endo/module-source that patches the native ModuleSource in place instead of entraining Babel. This will allow an XS-specific @endo/import-bundle variant to load both pre-compiled sources (pre-json-mjs) and original ESM sources (mjs).

This change is opt-in, provided the xs runtime condition. These are conditions introduced by Node.js with the node -C development entry.js precedent and supported by @endo/compartment-mapper for both endoScript and endoZipBase64 bundle formats.

Security Considerations

This change increases exposure to a native ModuleSource as implemented in a memory-unsafe language.

Scaling Considerations

We expect a significant improvement in performance at both bundle time and run time upon completion of #2251 since native ModuleSource is considerably faster than a virtualization.

Documentation Considerations

Captured in NEWS.md and README.md.

Testing Considerations

This change introduces a minimal text:xs that runs with xst and is exercised by Continuous Integration. This minimal test can be expanded as we make more progress with the XS-variant of SES, which will allow the virtual Compartment to opt-in to use native ModuleSource.

Compatibility Considerations

The XS-specific shim is not a one-to-one replacement for the Babel implementation and does not support some of the optional arguments. Code that expects these hooks to be called will be dismayed. Despite these limitations, no existing program should break: the xs condition should not be present in any existing application.

Upgrade Considerations

This change will not come to bear on the Agoric chain until after a new version of XS becomes available with a new XS-specific lockdown preamble.

@kriskowal kriskowal force-pushed the kriskowal-xs-module-source-shim branch 4 times, most recently from 2524d16 to 0e134ba Compare September 26, 2024 20:50
@kriskowal kriskowal force-pushed the kriskowal-xs-module-source-shim branch 3 times, most recently from 8ddbfc3 to 70beef1 Compare September 27, 2024 05:23
@kriskowal kriskowal force-pushed the kriskowal-xs-module-source-shim branch from 70beef1 to 5342ea3 Compare October 9, 2024 06:04
@kriskowal kriskowal force-pushed the kriskowal-xs-module-source-shim branch 19 times, most recently from 6375752 to a4b58b6 Compare October 18, 2024 22:53
@kriskowal kriskowal force-pushed the kriskowal-xs-module-source-shim branch 8 times, most recently from 555ea34 to 317c723 Compare October 19, 2024 05:52
@kriskowal
Copy link
Member Author

Notes from walk thru with @erights:

https://github.com/Moddable-OpenSource/moddable/blob/public/documentation/xs/XS%20Compartment.md#-module-binding

Add assertion to detect the XS bug for import "foo", which should not use { exportAllFrom } bindings.

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines 7 to 8
- Provides an XS-specific variant of `@endo/module-source` that adapts the
native `ModuleSource` instead of entraining Babel.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this under the # v1.1.0 heading rather than a new # Next release heading?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legitimate error. Thanks.

packages/module-source/shim.js Show resolved Hide resolved
/* global globalThis */
/* eslint-disable @endo/no-nullish-coalescing */

/** @typedef {{ import: string, as?: string, from: string } & { importAllFrom: string, as: string, from: string } & { export: string, as?: string, from?: string } & { exportAllFrom: string, as?: string } & {importFrom: string }} Binding */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Illegible. Please try something like

Suggested change
/** @typedef {{ import: string, as?: string, from: string } & { importAllFrom: string, as: string, from: string } & { export: string, as?: string, from?: string } & { exportAllFrom: string, as?: string } & {importFrom: string }} Binding */
/**
* @typedef {{ import: string, as?: string, from: string } &
* { importAllFrom: string, as: string, from: string } &
* { export: string, as?: string, from?: string } &
* { exportAllFrom: string, as?: string } &
* { importFrom: string }
* } Binding
*/

. But one that actually works. (I have no idea why the one above does not work.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it does seem to work for me. Or rather, it doesn't change how the code fails to work under the VSCode interactive type checker:

image

But both before and after pass yarn lint without even a warning, so I don't know why vscode isn't happy. (Btw, I did restart the TS Server, so it isn't that.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It likely needs to be a type union, not a type intersection.

Comment on lines +42 to +53
Object.defineProperties(
ModuleSource.prototype,
Object.getOwnPropertyDescriptors({
get imports() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach for adding accessors to an existing object. No change suggested.

@kriskowal kriskowal force-pushed the kriskowal-xs-module-source-shim branch 2 times, most recently from 3fe062f to 724fec7 Compare October 29, 2024 21:48
@kriskowal
Copy link
Member Author

I’ve addressed all feedback and am holding this back until we like it and #2471 as a set, perhaps even #2593 for stronger validation.

@kriskowal kriskowal force-pushed the kriskowal-xs-module-source-shim branch 3 times, most recently from fa10b9b to 9e16181 Compare October 30, 2024 22:55
@kriskowal kriskowal force-pushed the kriskowal-xs-module-source-shim branch from 9e16181 to 53e5109 Compare November 14, 2024 21:02
@kriskowal kriskowal enabled auto-merge November 14, 2024 21:12
@kriskowal kriskowal merged commit b3f0c56 into master Nov 14, 2024
15 checks passed
@kriskowal kriskowal deleted the kriskowal-xs-module-source-shim branch November 14, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export native XS ModuleSource in lieu of StaticModuleRecord on XS
3 participants