WICG / import-maps

How to control the behavior of JavaScript imports
https://html.spec.whatwg.org/multipage/webappapis.html#import-maps
Other
2.66k stars 69 forks source link

Depcache spec and reference implementation #210

Open guybedford opened 4 years ago

guybedford commented 4 years ago

This PR implements the "depcache" proposal in https://github.com/WICG/import-maps/issues/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

marcoscaceres commented 4 years ago

@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 commented 4 years ago

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:

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 commented 4 years ago

@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 commented 4 years ago

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

guybedford commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

guybedford commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

Ah, ok! if import-maps is mostly "done" per https://github.com/WICG/import-maps/pull/210#issuecomment-590918055 - and more or less ready to migrate out of the WICG, then yes, I agree with @domenic forking would be appropriate here.