-
Notifications
You must be signed in to change notification settings - Fork 70
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
refactor(ses): Retool for module descriptors #2321
Conversation
176f710
to
4d3a95b
Compare
0a6fbb9
to
62fe754
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.
I probably need a live discussion before I'm knowledgeable enough to complete the review. But a lot already LGTM
194959e
to
2d7ace2
Compare
// A (legacy) behavior: If we do not recognize the module descriptor as a | ||
// module descriptor, we assume that it is a module source (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.
At some point, we’ll want to add options for strict portability and this case will become an exception in that mode. We would stop trying to differentiate module sources from module descriptors.
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'm fine with the code here in this PR. But I don't understand the comment. Why not continue to differentiate module sources from module descriptors?
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.
We’ve discriminated all the module descriptors up to this point, but to preserve backward compatibility with import hooks that simply returned a module source (not a module source wrapped in a module descriptor), we have to fall through here.
We do have a clear path to deprecate this path, though, which I hope to get to separately.
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.
which I hope to get to separately
Worth filing an issue?
caf80a9
to
db465e4
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.
Sorry it took so long.
LGTM, thanks!
packages/ses/src/module-load.js
Outdated
} | ||
if (typeof aliasNamespace === 'string') { | ||
|
||
if (typeof moduleDescriptor === 'string') { | ||
// eslint-disable-next-line @endo/no-polymorphic-call | ||
assert.fail( |
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.
On the function declaration, I'm currently getting the lint warning
131:11 warning Expected to return a value at the end of generator function 'loadWithoutErrorAnnotation' consistent-return
but if I change this line to
assert.fail( | |
throw assert.fail( |
and (see below)
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 presumably means that the type isn’t getting properly threaded, since fail
and Fail
return never
. I’ll change this to throw assert.error
since throwing at an inevitably throwing call feels weird.
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 felt weird for me the first 100 times. But our codebase now does that pervasively. In throw Fail...
I read the Fail
as "throw an error" and the throw
as "Let static analysis know that we're throwing an error".
packages/ses/src/module-load.js
Outdated
moduleLoads, | ||
); | ||
} else { | ||
Fail`module descriptor must be a string or object for specifier ${q( |
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.
(continued from above)
and modify this line to
Fail`module descriptor must be a string or object for specifier ${q( | |
throw Fail`module descriptor must be a string or object for specifier ${q( |
the warning goes away.
packages/ses/test/import.test.js
Outdated
moduleMapHook(specifier) { | ||
if (specifier === './index.js') { | ||
return new StaticModuleRecord('export default 42'); | ||
} | ||
}, |
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 get 7 consistent return
lint warnings in this file.
moduleMapHook(specifier) { | |
if (specifier === './index.js') { | |
return new StaticModuleRecord('export default 42'); | |
} | |
}, | |
moduleMapHook(specifier) { | |
if (specifier === './index.js') { | |
return new StaticModuleRecord('export default 42'); | |
} | |
return undefined; | |
}, |
// A (legacy) behavior: If we do not recognize the module descriptor as a | ||
// module descriptor, we assume that it is a module source (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.
I'm fine with the code here in this PR. But I don't understand the comment. Why not continue to differentiate module sources from module descriptors?
db465e4
to
5df0f1a
Compare
Refs: #400 #2251
Description
Pursuant to arriving at parity with XS, this change internally reörients the SES module loader around the concept of a “module descriptor”. This creates some clarity and removes some existing duplication of work for “alias” module descriptors, and prepares the material for a richer module descriptor schema to match those implemented by XS.
Security Considerations
No changes.
Scaling Considerations
No changes.
Documentation Considerations
No changes.
Testing Considerations
No changes.
Compatibility Considerations
No changes.
Upgrade Considerations
No changes.