-
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(compartment-map): Decouple discovery and languages #2306
Conversation
756c062
to
1d02471
Compare
Advise reviewing commits individually. I took some care to rename and edit in separate steps. |
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.
if I'm being fussy, I'd want to see, for each of these new files, a module-level comment describing the module's use-case. and descriptions for each of the exported functions.
probably needs a NEWS.md
update.
there's nothing here that I'd block about.
@@ -28,8 +28,14 @@ | |||
"default": "./index.js" | |||
}, | |||
"./import.js": "./import.js", | |||
"./import-lite.js": "./import-lite.js", |
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.
(suggestion/observation/trolling) there's obviously a convention at play here, but I'd omit the .js
from the keys
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 very much want to. It’s not just convention, though. This makes the Compartment Mapper continue to work when used on some older foundations like node -r esm
that do not read the "exports"
directive, so we can’t use it for aliases, just to limit exposure. We have one lingering integration test that needs to be replaced before we can drop this 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.
Didn't realize that esm
was a target
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.
Would it be reasonable to provide both extension-laden and extension-free exports?
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 think that’s reasonable when after eliminate standardthings/esm
from our toolchain. Until then, better to encourage usage patterns that are portable.
// Have to give it a name to capture the external meaning of Compartment | ||
// Otherwise @param {typeof Compartment} takes the Compartment to mean | ||
// the const variable defined within the function. | ||
// |
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.
(suggestion) IMO it isn't necessary to explain this, but nbd. I would probably even just zap the @typedef
altogether and use typeof Compartment
in the parseArchive
signature's docstring
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.
Using typedef Compartment
doesn’t work for some reason, thus the comment. My preference as well.
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.
@kriskowal FWIW I cannot reproduce whatever the problem was that you ran into. This compiles cleanly for me:
/**
* @typedef {object} Options
* @property {string} [expectedSha512]
* @property {HashFn} [computeSha512]
* @property {Record<string, unknown>} [modules]
* @property {ExitModuleImportHook} [importHook]
* @property {typeof Compartment} [Compartment]
* @property {ComputeSourceLocationHook} [computeSourceLocation]
* @property {ComputeSourceMapLocationHook} [computeSourceMapLocation]
* @property {ParserForLanguage} [parserForLanguage]
*/
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.
that said, it is a little weird that we are expecting the Compartment
constructor here. there is only one Compartment
constructor. is there a place where we're actually using a different one?
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.
Yeah, there are Compartment
compatible constructors created by wrapping the Compartment
constructor. Notably, Compartment
before lockdown is different than after. Every Compartment
has its own instance of Compartment
. They’re structurally compatible, though. Unclear to me why that would make a difference to types, but they do have different values.
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.
even though TS is structural, because Compartment
is a function, it will only ever allow the global Compartment
from whichever version of ses
provides it. That may be fine, but also seems redundant. Though it may be useful if we're using some mocked-out Compartment
in tests? even then it seems like it would be the same Compartment
constructor
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.
src/import-archive-lite.js:266:4 - error TS2502: 'Compartment' is referenced directly or indirectly in its own type annotation.
266 * @param {typeof Compartment} [options.Compartment]
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.
ah, I see that now. It's because of the DefaultCompartment
. typeof DefaultCompartment
works
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.
if I'm being fussy, I'd want to see, for each of these new files, a module-level comment describing the module's use-case. and descriptions for each of the exported functions.
probably needs a NEWS.md
update.
there's nothing here that I'd block about.
27e7d9c
to
68ddb71
Compare
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.
mo tags mo problems
@@ -28,8 +28,14 @@ | |||
"default": "./index.js" | |||
}, | |||
"./import.js": "./import.js", | |||
"./import-lite.js": "./import-lite.js", |
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.
Didn't realize that esm
was a target
68ddb71
to
3872e35
Compare
3872e35
to
d241fcb
Compare
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.
Refs: #400 and #2294
Description
In this change, we carve and export
-lite.js
and-parsers.js
out ofimport.js
,archive.js
, andimport-archive.js
, as well revealingmapNodeModules
through@endo/compartment-mapper/node-modules.js
revealing hidden flexibility already present in the Compartment Mapper. This allows the Compartment Mapper to mix and match “language behaviors” with different workflows (e.g., usingimport-parsers.js
witharchive-lite.js
instead ofarchive-parsers.js
generates archives with original sources instead of precompiled sources.) We also decouple the process of discovering the physical compartments such thatnode-modules.js
can be substituted for alternate compartment exploration algorithms, e.g., combining a custom lockfile and package cache from a specific package management solution.Security Considerations
Does not cross security considerations.
Scaling Considerations
Exporting more narrowly scoped modules allows consumers to be avoid entraining Babel in a greater variety of scenarios.
Documentation Considerations
Evolution of the reference documentation should suffice, though another pass at README.md to dig into these more surgical and composable APIs may be worthwhile.
Testing Considerations
The existing scenarios are extensive and fully cover the behaviors that we’ve factored out. I expect #2294 tests to cover the new scenarios.
Compatibility Considerations
This change takes great care to preserve the existing behavior of the composite
import.js
,archive.js
, andimport-archive.js
interfaces, only revealing existing behaviors that were previously internal, allowing more expressible scenarios.Upgrade Considerations
This change will not affect upgrade and paves a migration path forward for preserving backward-compatibility for
bundleSource
andimportBundle
as we draw closer to supporting XS native compartments.