feat(ses): Support importHook alias returns #432
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kriskowal
force-pushed
the
kris/ses-compartment-names
branch
from
3a12559
to
68f2852
Compare
kriskowal
force-pushed
the
kris/ses-import-hook-alias
branch
from
09d86bc
to
53caadf
Compare
kriskowal
force-pushed
the
kris/ses-compartment-names
branch
from
68f2852
to
e140f2c
Compare
kriskowal
force-pushed
the
kris/ses-import-hook-alias
branch
from
53caadf
to
40fd429
Compare
kriskowal
force-pushed
the
kris/ses-compartment-names
branch
from
e140f2c
to
ec31d51
Compare
kriskowal
force-pushed
the
kris/ses-import-hook-alias
branch
2 times, most recently
from
a34836d
to
e5e6609
Compare
kriskowal
force-pushed
the
kris/ses-compartment-names
branch
from
4f9b18a
to
cf22857
Compare
kriskowal
force-pushed
the
kris/ses-import-hook-alias
branch
from
e5e6609
to
89c12b5
Compare
kriskowal
force-pushed
the
kris/ses-compartment-names
branch
from
cf22857
to
abc3697
Compare
kriskowal
force-pushed
the
kris/ses-import-hook-alias
branch
from
89c12b5
to
7dd6f96
Compare
kriskowal
force-pushed
the
kris/ses-compartment-names
branch
from
abc3697
to
ef542c7
Compare
kriskowal
force-pushed
the
kris/ses-import-hook-alias
branch
from
7dd6f96
to
ade51a6
Compare
erights
approved these changes
Aug 26, 2020
packages/ses/README.md
Outdated
Comment on lines
+186
to
+187
| and return a module that has a different "response specifier" than the original | ||
| "request specifier". |
There was a problem hiding this comment.
I really like the terminology "request specifier" and "response specifier"
packages/ses/README.md
Outdated
|
|
||
| ```js | ||
| const importHook = async specifier => { | ||
| const candidates = [specifier, specifier + ".js", specifier + "/index.js"]; |
There was a problem hiding this comment.
Suggested change
| const candidates = [specifier, specifier + ".js", specifier + "/index.js"]; | |
| const candidates = [specifier, `${specifier}.js`, `${specifier}/index.js`]; |
Only if you agree it is at least as clear.
packages/ses/README.md
Outdated
| const importHook = async specifier => { | ||
| const candidates = [specifier, specifier + ".js", specifier + "/index.js"]; | ||
| for (const candidate of candidates) { | ||
| const record = wrappedImportHook(candidate).catch(_ => undefined); |
packages/ses/README.md
Outdated
Comment on lines
+188
to
+189
| The `importHook` may return an "alias" envelope with `alias`, `compartment`, | ||
| and `module` properties. |
There was a problem hiding this comment.
The example code below instead returns a record with alias and specifier properties?
What does "envelope" mean here?
There was a problem hiding this comment.
Revised to avoid “envelope” term.
packages/ses/README.md
Outdated
Comment on lines
+236
to
+242
| const moduleMapHook = moduleSpecifier => { | ||
| if (moduleSpecifier === 'even') { | ||
| return even.module('./index.js'); | ||
| } else if (moduleSpecifier === 'odd') { | ||
| return odd.module('./index.js'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Our old even/odd friend, I missed you.
packages/ses/src/module-load.js
Outdated
| // Await all dependencies to load, recursively. | ||
| await Promise.all( | ||
| values(resolvedImports).map(fullSpecifier => | ||
| // Behold: recursion. |
There was a problem hiding this comment.
I wish "Behold: recursion." was the eslint directive ;)
Actually, I'll take this opportunity to ask: Do you think the Agoric style should turn this warning off by default?
There was a problem hiding this comment.
It’s a rare enough problem that it doesn’t bother me.
I don’t think I’ve ever used recursion accidentally, though.
The directive does force an opinionated order. In this particular case, these functions tend to tail-recur so they’re actually clearer to read in reverse declaration order.
I have no strong feelings.
packages/ses/test/import.test.js
Outdated
|
|
||
| const makeImportHook = makeNodeImporter({ | ||
| 'https://example.com/main/index.js': ` | ||
| export const unique = {n: Math.random()}; |
There was a problem hiding this comment.
Really vivid way to test this. I like it.
packages/ses/test/import.test.js
Outdated
|
|
||
| // TODO The following commented test does not pass, and might not be valid. | ||
| // Web browsers appear to have taken the stance that they will load a static | ||
| // module record once per *response url* and create unique a unique module |
There was a problem hiding this comment.
Suggested change
| // module record once per *response url* and create unique a unique module | |
| // module record once per *response url* and create a unique module |
kriskowal
force-pushed
the
kris/ses-import-hook-alias
branch
2 times, most recently
from
cf7b21e
to
8b4bee3
Compare
This change enables importHook to return a record for a module that has a different specifier than the requested module specifier. This enables the Node.js pattern of redirecting `name` to `name/index.js` and respecting the latter name as the referrer for its own imports.
kriskowal
force-pushed
the
kris/ses-compartment-names
branch
from
2bdad74
to
aa7871e
Compare
kriskowal
force-pushed
the
kris/ses-import-hook-alias
branch
from
8b4bee3
to
93a7f21
Compare
kriskowal
added a commit
that referenced
this pull request
Aug 26, 2020
This change enables importHook to return a record for a module that has a different specifier than the requested module specifier. This enables the Node.js pattern of redirecting `name` to `name/index.js` and respecting the latter name as the referrer for its own imports.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change enables importHook to return a record for a module that has a different specifier than the requested module specifier. This enables the Node.js pattern of redirecting
nametoname/index.jsand respecting the latter name as the referrer for its own imports.