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(static-module-record): add support for import.meta #1141

Merged
merged 6 commits into from
Jun 14, 2022

Conversation

naugtur
Copy link
Member

@naugtur naugtur commented Apr 5, 2022

Implemented import.meta according to #291

The actual implementation of exposing import.meta.url to modules loaded by compartment-mapper is going to be a separate PR.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Let’s talk requirements for this feature at Endo Sync tomorrow. The Compartment abstraction will be aware of meta but not meta.url. For that, we need to give the hooks system a way to populate meta. The current notion is that the import/loadHook will be able to return an object with a meta property and the compartment will copy the properties to the meta object. Because some of those properties are expensive, we would have an importMetaHook that takes a meta object and add things like resolve but would only be called if a module utters import.meta in its source text.

@kriskowal
Copy link
Member

See #291

@naugtur
Copy link
Member Author

naugtur commented May 17, 2022

Made some progress on the implementation. Looks like it's it.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Yeah, this is on-track. The comments I’ve left pertain to the leading half of this feature, which needs to allow for usage like:

const c = new Compartment({}, {}, {
  importHook(specifier) {
    const requestUrl = locate(specifier);
    const response = await fetch(requestUrl);
    const responseText = await response.text();
    return {
      record: new StaticModuleRecord(responseText),
      meta: {url: responseURL};
    };
  },
  importMetaHook(meta) {
    meta.resolve = url => importMetaResolve(url, meta.url);
  },
});

packages/compartment-mapper/src/bundle.js Outdated Show resolved Hide resolved
packages/ses/test/test-import.js Outdated Show resolved Hide resolved
packages/ses/src/module-instance.js Outdated Show resolved Hide resolved
packages/ses/src/module-instance.js Outdated Show resolved Hide resolved
packages/static-module-record/src/transform-analyze.js Outdated Show resolved Hide resolved
@naugtur naugtur force-pushed the naugtur-import-meta-url branch 2 times, most recently from d34b909 to dfedd96 Compare May 18, 2022 13:05
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Nice tests!

packages/ses/src/module-instance.js Outdated Show resolved Hide resolved
packages/ses/src/module-instance.js Outdated Show resolved Hide resolved
packages/ses/src/module-load.js Outdated Show resolved Hide resolved
packages/static-module-record/src/static-module-record.js Outdated Show resolved Hide resolved
packages/ses/README.md Outdated Show resolved Hide resolved
@naugtur naugtur force-pushed the naugtur-import-meta-url branch 2 times, most recently from 4da14ac to 4f7911b Compare May 26, 2022 12:24
@naugtur naugtur marked this pull request as ready for review May 26, 2022 12:25
@naugtur naugtur requested a review from kriskowal May 26, 2022 12:25
@naugtur naugtur changed the title feat(static-module-record): add support for import.meta.url feat(static-module-record): add support for import.meta May 26, 2022
@naugtur
Copy link
Member Author

naugtur commented May 26, 2022

@kriskowal Unless I need to put more contrnt into packages/ses/README.md I think this is ready for final review.

@naugtur naugtur force-pushed the naugtur-import-meta-url branch 3 times, most recently from 3b5d364 to c176b04 Compare June 7, 2022 10:42
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

This is ready to land, though the specification it’s based on needs a little more time to bake. Until then, I’ve left some notes for your consideration.

packages/ses/README.md Outdated Show resolved Hide resolved
packages/ses/src/module-instance.js Outdated Show resolved Hide resolved
packages/static-module-record/src/hidden.js Outdated Show resolved Hide resolved
packages/static-module-record/src/static-module-record.js Outdated Show resolved Hide resolved
packages/static-module-record/src/transform-analyze.js Outdated Show resolved Hide resolved
packages/static-module-record/src/transform-analyze.js Outdated Show resolved Hide resolved
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

…and I’m leaving a stamp so you don’t have to wait for my blessing when everything’s ready.

@naugtur naugtur force-pushed the naugtur-import-meta-url branch 3 times, most recently from d61c012 to ce2caee Compare June 13, 2022 11:08
…ns in exports + benchmark setup (#1188)

* babelPlugin fix proposal and benchmarking setup

* remove debris

Co-authored-by: Michael FIG <mfig@agoric.com>

* more cleanup
@naugtur naugtur merged commit 35d9489 into master Jun 14, 2022
@naugtur naugtur deleted the naugtur-import-meta-url branch June 14, 2022 08:48
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.

None yet

2 participants