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

Depcache spec and reference implementation #210

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

guybedford
Copy link
Collaborator

@guybedford guybedford commented Feb 22, 2020

This PR implements the "depcache" proposal in #209, providing an optimal solution to the waterfall problem, while ensuring that import maps remain the source of truth for the graph and optimization for tools that generate them.

Let's keep proposal feedback to the issue, and direct spec feedback to this PR.

The approach I've implemented here is to effectively integrate the depcache check into fetch a single module script in the HTML spec, where the recursion between this algorithm calling preload depcache, which in turn is calling fetch a single module script perform the graph traversal. This traversal is very simple and naturally stops on unnecessary iterations due to the module map existence checks, avoiding the need for visited lists and such.

To support the new resolve a module specifier signature, I've separated out the top-level call to it from HTML from the base-level API-like call to its resolution, called resolve import maps, which is just takes a specifier, import map and baseURL. This way the depcache preload can easily call into that resolution. Error handling has been carefully specified as well here.

Any changes to the fetch a single module script signature in further refactoring can be applied equally to the depcache preload algorithm, although it is worth noting that the script for depcache preloads doesn't exist at the time of the network request.

Feedback welcome, happy to keep working on this.


Preview | Diff

@guybedford guybedford changed the title Depcache spec and reference implementation proposal Depcache spec and reference implementation Feb 22, 2020
@marcoscaceres
Copy link
Contributor

@guybedford, thanks! We will need you to join the WICG to merge this. You should have an invite in your inbox. Let me know once that is done, and then we can merge this.

@hiroshige-g
Copy link
Collaborator

Brainstorming some issues (not hard, so I expect we can figure out solutions later; right now I don't have bandwidth though):

I prefer not to call #preload-a-dependency-graph-cache from fetch a single module script, because:

  • In the spec, fetch a single module script is, as indicated by the name, only for loading a single module, while the callers of fetch a single module script is responsible for traversing module graphs. This separation keeps Chromium implementation cleaner, so I'd like to keep this separation.
  • Also, fetch a single module script is an internal helper concept in the spec. Embedding the preloading behavior within such an internal helper (and in the form of mutually recursive calls) might be unclear.

Also, I suppose the current PR uses module map to avoid infinite loops (i.e. if moduleMap[urlString] is already set, we don't preload further), but this is different from how module graph loading avoids infinite loops (e.g. https://html.spec.whatwg.org/multipage/webappapis.html#fetch-the-descendants-of-a-module-script uses visited set). I feel having a single source of infinte loop avoidance is preferrable for long-term code health.

@guybedford
Copy link
Collaborator Author

@marcoscaceres thanks for the info here. I should already be a member I believe? I was sent an email to link my GitHub profile, which I did after posting this PR, but the check hasn't seemed to have updated. Any tips on further steps I can take here welcome.

@domenic
Copy link
Collaborator

domenic commented Feb 24, 2020

I think this might be best in a separate repository, so it can gather its own community and interested implementers.

@guybedford
Copy link
Collaborator Author

@domenic I'm a little weary of aiming to get interest for a spec on top of a spec on top of a spec. The calls from such a depcache spec to WICG import maps will be quite hand-wavey with some complex interactions. In addition I did need to perform some injection and refactoring to the existing import maps algorithms. If you think this is the best approach, then I think a fork might be preferable, with the intent to re-merge. I would just like to see SystemJS and ES module shims built to spec work here on the optimization problems which is my primary interest as an implementor, so this seems like the right sort of route to me. Would you be ok with that?

@domenic
Copy link
Collaborator

domenic commented Feb 24, 2020

I mean, I have no objection to you forking. I just think it's important to separate the import maps community in this repository from any additional proposals, which have different levels of support. I'm OK with keeping one issue open (e.g. either #208 or #209) to direct folks from this community into that separate body of work, but I don't think it's a good idea to accept proposals like this into the import maps spec without (browser) implementer interest.

@marcoscaceres
Copy link
Contributor

@guybedford thanks! The IPR-checking system has found you :) all green.

@guybedford
Copy link
Collaborator Author

guybedford commented Feb 25, 2020

@domenic is there any way you could still be convinced this is a useful feature for this group to consider? Have you followed the issue discussion? I just haven't really heard a real argument from you for why you don't think this feature is suitable for this spec. Personally I see this as a critical feature for production modules, and first opened an issue on this problem two years ago. I'm happy to assist with spec work, implementation work and even creating a Chromium CL for it.

@domenic
Copy link
Collaborator

domenic commented Feb 25, 2020

I'm just looking to finish import maps. I am sure there are a ton of useful features in the modules space, of varying criticality in different peoples' judgments. I think they need to build their own communities and support. (I have moved on from working on modules, so import maps will be my last project in that space.)

@littledan
Copy link
Contributor

@hiroshige-g I'm sympathetic with the aim of simplifying/unifying recursion and working with cycles (it's complicated on the JS side too!), but it looks like if this preloading happened during "fetch the descendants of a module script", it would happen after the fetch has actually completed and we have the whole module, which seems unnecessarily delayed. Was that the intention?

@marcoscaceres
Copy link
Contributor

Ah, ok! if import-maps is mostly "done" per #210 (comment) - and more or less ready to migrate out of the WICG, then yes, I agree with @domenic forking would be appropriate here.

@yoavweiss yoavweiss changed the base branch from master to main March 5, 2021 21:39
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.

5 participants