-
Notifications
You must be signed in to change notification settings - Fork 74
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(ses): Module descriptors for XS parity #2345
Conversation
2a29262
to
d3d8b9f
Compare
29e3f8d
to
3147a5d
Compare
d3d8b9f
to
d3b447f
Compare
3147a5d
to
319c8e1
Compare
d3b447f
to
91c3cd3
Compare
319c8e1
to
8c74126
Compare
91c3cd3
to
caf80a9
Compare
8c74126
to
c7fdfb2
Compare
caf80a9
to
db465e4
Compare
c7fdfb2
to
6320908
Compare
6320908
to
68d8f93
Compare
packages/ses/README.md
Outdated
|
||
```js | ||
import 'ses'; | ||
import { StaticModuleRecord } from '@endo/static-module-record'; | ||
import { StaticModuleRecord as ModuleSource } from '@endo/static-module-record'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this renaming into @endo/static-module-record , if necessary still continuing to export the old name for now, but deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, could we also rename the package from static-module-record to module-source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll follow-up. This is a good time.
default `importMeta` object is an empty object. | ||
|
||
Compartments copy the `importMeta` object properties into the module | ||
`import.meta` object like `Object.assign`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it actually use Object.assign
semantics, as in https://github.com/endojs/endo/blob/master/packages/pass-style/doc/enumerating-properties.md ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copypasta from XS docs. We don’t actually implement importMeta
yet, but if we did, we’d use Object.assign
.
|
||
- Otherwise, the value of `namespace` property must be an object. The module is | ||
loaded and initialized from the object according to the [virtual module | ||
namespace](#VirtualModuleNamespace) pattern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does #VirtualModuleNamespace link to? I don't see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s an internal anchor to the corresponding heading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is that corresponding heading? I did not see it.
packages/ses/README.md
Outdated
'c2': c2.module('./main.js'), | ||
const compartment = new Compartment({}, { | ||
c: { | ||
source: new StaticModuleRecord(''), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source: new StaticModuleRecord(''), | |
source: new ModuleSource(''), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do a global search for stale names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll do a follow-up.
packages/ses/README.md
Outdated
const moduleText = fs.readFileSync(moduleLocation); | ||
return new StaticModuleRecord(moduleText, moduleLocation); | ||
return { | ||
source: new StaticModuleRecord(moduleText, moduleLocation), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source: new StaticModuleRecord(moduleText, moduleLocation), | |
source: new ModuleSource(moduleText, moduleLocation), |
packages/ses/src/module-load.js
Outdated
aliasDescriptor = weakmapGet(moduleAliases, namespace); | ||
if (aliasDescriptor !== undefined) { | ||
moduleDescriptor = aliasDescriptor; | ||
// Fall through to processing the reesulting {compartment, specifier} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Fall through to processing the reesulting {compartment, specifier} | |
// Fall through to processing the resulting {compartment, specifier} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The README.md is an awesome guide for stating this stuff precisely. Should already be much of the work towards spec text!
Credit where due https://github.com/Moddable-OpenSource/moddable/blob/public/documentation/xs/XS%20Compartment.md#-module-descriptor |
db465e4
to
5df0f1a
Compare
68d8f93
to
984d3d2
Compare
debd810
to
5998c1b
Compare
5998c1b
to
18796bc
Compare
Refs: #400 #2251
Description
This change introduces support for XS-compatible module descriptors with the discriminating properties
source
andnamespace
, including support for loading a module from the parent compartment.Security Considerations
None.
Scaling Considerations
None.
Documentation Considerations
This change is additive.
Testing Considerations
Covered.
Compatibility Considerations
Some usage patterns are now deprecated but continue to be supported without complaint.
Upgrade Considerations
None.