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

Making "resolve a module specifier" async? #18

Closed
hiroshige-g opened this issue Mar 16, 2018 · 7 comments
Closed

Making "resolve a module specifier" async? #18

hiroshige-g opened this issue Mar 16, 2018 · 7 comments

Comments

@hiroshige-g
Copy link
Collaborator

(Related to #6)

If a package name map is being requested, then fetching of bare modules waits for the package name map fetch.

I feel this would require resolve a module specifier to be async, so that it can wait until package name load completion.

My random thoughts:

(I feel Option 1 and Option A look good but haven't investigated the issues around them thoroughly; also there can be other options)

Called from fetch the descendants of a module script

Option 1

  • Make resolve a module specifier async.
  • Make internal module script graph fetching procedure to
    • take a specifier (instead of url) as an argument,
    • call resolve a module specifier asynchronously (instead from fetch the descendants of a module script Step 5),
    • then check visited set (instead in fetch the descendants of a module script Step 5),
    • then proceed to Step 2.

pros:

  • As internal module script graph fetching procedure spec is written in a highly async fashion, I expect the changes to spec and implementation can be done relatively easy, without breaking user-visible behavior.
  • This will allow fetch a module script graph to take specifier as well, i.e. allow <script> to take specifier.

cons:

  • This still requires structural change (one additional async step in internal module script graph fetching procedure, move visited set across spec concepts), and thus changes to spec/implementation will be non-trivial.
  • Especially, we have to be very careful about moving visited set check (e.g. to avoid causing performance/non-determinism issues again).

Option 2

  • Make resolve a module specifier async.
  • Just update fetch the descendants of a module script around Step 5 to allow resolve a module specifier to be async, i.e. make Step 5 to wait for all resolve a module specifier calls and then proceed to Step 6.

pros:

  • Smaller changes.
  • quite simple, easier to reason.

cons:

  • Slows down non-bare specifiers when a module script contains both non-bare and bare specifiers.

We might want to start internal module script graph fetching procedure for child module scripts with non-bare specifiers, and wait for all other bare-specifiers resolution and start internal module script graph fetching procedure for child module scripts with bare specifiers.
However, in this case I expect we still have to reason about visited set check changes, and have to consider similar things to Option 1.

Option 3

  • Keep resolve a module specifier sync.
  • Make resolve a module specifier return URLs if successfully resolved without waiting for map loading,
    and return bare specifier as-is if package name file is still loading, and let the subsequent internal module script graph fetching procedure to wait/block for actual resolution.

pros:

  • resolve a module specifier is sync.

cons:

  • Reasoning is harder than Option 1/2, due to mixture of URLs and bare-specifiers returned by resolve a module specifier. If we make resolve a module specifier always return bare-specifiers, then it would be virttually the same as Option 1.

Called from create a module script

Option A

Still require create a module script to throw an error if the specifier is wrong (for bare specifiers, there's no package map, or resolution with the package map fails).

pros:

The semantics for users will be clear.

cons:

Have to make create a module script async (unless Option 3 is selected), which looks a little awkward. (Not so hard, as create a module script is basically in the middle of async call chains in the spec; not so sure about implementation changes though)

Option B

Just do some sanity checks in create a module script (i.e. do not check bare specifiers if the package map is still loading).

pros:

Implementation might be easier?

cons:

Whether create a module script throws or not becomes non-deterministic.
Exposure of this non-determinism to users can be prevented by tweaking find the first parse error, but this non-determinism makes reasoning harder and I'm not sure whether this non-determinism is exposed to users in corner cases.

@nyaxt
Copy link

nyaxt commented Mar 19, 2018

Note: This issue is blocking Blink implementation

I prefer Option 1 & A proposed here.

@bicknellr
Copy link

Assuming that an import.meta.resolve(importUrl) -> networkUrl ends up replacing the 'active script' idea, but this function is async, I think it would make a lot of currently simple situations more complicated:

class FancyImage extends HTMLElement {
  constructor() {
    super();
    this.attachShadow({mode: 'open'}).innerHTML = `
      <style>
      :host { display: contents; }
      img { border: 5px dashed magenta; } /* so fancy */
      </style>
      <img src="What goes here?">
    `;
  }
}
customElements.define('fancy-image', FancyImage);

If import.meta.resolve exists and is sync, then you can just insert it into your template:

<img src="${import.meta.resolve("import:other-package/desired-image.png")}">

If import.meta.resolve is async, now you have to wait for the URL to resolve, find the node, and set the property:

import.meta.resolve("import:other-package/desired-image.png").then(networkUrl => {
  this.shadowRoot.querySelector("img").src = networkUrl;
});

Also, the connection between the HTML used to initialize the shadow root's DOM and the import: URLs used within in them is lost, which seems like it would be confusing as the number of import: URLs you use grows.

This is more awkward if you need to inline URLs into something like CSS:

class OtherFancyThing extends HTMLElement {
  constructor() {
    super();
    this.attachShadow({mode: 'open'}).innerHTML = `
      <style>
      @import "What goes here?";
      /* more CSS */
      </style>
      <!-- more HTML -->
    `;
  }
}
customElements.define('other-fancy-thing', OtherFancyThing);

Should I wait for the URL to resolve first and only add my styles asynchronously?

const generateStyles = (importUrl) => {
  const style = document.createElement('style');
  style.innerHTML = `
    @import "${importUrl}";
    /* more CSS */
  `;
  return style;
};

class OtherFancyThing extends HTMLElement {
  constructor() {
    super();
    this.attachShadow({mode: 'open'}).innerHTML = `
      <!-- more HTML -->
    `;
    import.meta.resolve("import:my-common-styles/text.css").then(networkUrl => {
      this.shadowRoot.appendChild(generateStyles(networkUrl));
    });
  }
}
customElements.define('other-fancy-thing', OtherFancyThing);

@hiroshige-g
Copy link
Collaborator Author

Closing as Fixed.

In the current draft spec, we chose to call wait for import maps before/outside resolve a module specifier, and thus resolve a module specifier no longer needs to wait for pending import maps and remains synchronous, even with external import maps.

As for another asynchronous factor, there can be "fallback from network" (e.g. HTTPS->HTTPS fallback) in the future, but even in that case, resolve a module specifier would resolve specifiers to import: URLs synchronously. (I think there are some other issues around that, but I'll anyway track them separately.)

The issue whether import.meta.resolve() can/should be async is now tracked by #79.

@guybedford
Copy link
Collaborator

It's worth noting that when it comes to dynamically loading import maps after the page load, this is something that might cause the resolver to wait for import maps as well, so I don't think we should consider this aspect of the specification "closed off" for further conceptualizing.

@guybedford
Copy link
Collaborator

I guess such a wait for import maps being caused to delay dynamically for subsequent dynamic import maps could still continue happen at the top-level loading part of either the <script type="module"> injection or dynamic import as it does currently though actually?

@hiroshige-g
Copy link
Collaborator Author

Currently I don't allow new dynamically added import maps after any module loading started, so I assume at the time of resolve-a-module-specifier all import maps are already loaded and immutable.

Even when we want to allow new import maps after module loading started (#92 ), I'm not sure whether resolve a module specifier itself should wait for import maps -- and we should resolve other more fundamental & harder issues for #92 before considering this kind of aspects. Let's reopen this issue when this turns actually needed again.

@guybedford
Copy link
Collaborator

We may need to carefully design a dynamic load phase to start after all current in-progress modules have completed or failed (so that their intermediate resolutions are not affected), and have that dynamic load phase also wait / block any new module loads. But certainly let's track the use case at #92 further.

It is important to note though that how such a phase is handled does affect whether import.meta.resolve should be async or not. Because having the wait on import maps there would in theory make import.meta.resolve being called during the dynamic load phase more predictable in that it would wait on the latest import maps information.

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

No branches or pull requests

5 participants