Conversation
|
(Note the preload implementation is based off of old SystemJS which was designed to support browsers at the time... this is probably a good thing for compat, but full current browser support still needs to be verified!) |
internettrans
left a comment
There was a problem hiding this comment.
This is looking good to me - I wasn't aware of the new Image() approach to preloading in older browsers so that was fun to learn.
src/features/import-map.js
Outdated
|
|
||
| const systemInstantiate = systemJSPrototype.instantiate; | ||
| systemJSPrototype.instantiate = function (url, firstParentUrl) { | ||
| const depcache = importMap.depcache[url]; |
There was a problem hiding this comment.
Shouldn't this be done recursively for the whole dependency tree? Otherwise, we're just halving the waterfall effect, no?
There was a problem hiding this comment.
The behavior here matches Guy's proposal to the import map spec
I agree with his proposed spec - eagerly prefetching all of the modules in the import map is fairly irresponsible use of the network. It would result in a lot of unnecessary requests, which is stated in the proposal as something to avoid.
internettrans
left a comment
There was a problem hiding this comment.
This looks good to me. Using getOrCreateLoad for deep recursion is a nice solution. Is there anything remaining on this, besides tests?
|
I don't want to land this until it is clear that this is a direction we want to encourage. I'm still doing perf tests on various real-world use cases. Until the major benefit can be seen I'll leave this up as a WIP. |
|
I can't wait to use this feature!
In order to share "react" between "top-app" and "external-lib", I am forced to build react as a separate systemjs bundle:
Separating "react" lead to a loading waterfall:
What I want is this:
As you can see, I can use multiple import to achieve parallel loading, but depcache can make this scalable. Another question: Seeing that "top-app.system.js" and "react.system.js" is critical resouce for the page, can I inline "top-app.system.js" and "react.system.js" into HTML? So that I can further reduce one request ?
I ask this question in another issue: #2148 (comment) |
|
Driving any spec response on depcache has completely stalled. But I'd still like to move towards making this a first-class SystemJS feature as it always was. Does any one object to me merging this over the coming weeks? Any implementation concerns? |
|
(I'm going to check the numbers again on some larger projects with the goal to merge) |
|
I don't have any problems with this implementation |
coreFile by file impact
extrasFile by file impactPull request have no impact on |
|
Released in 6.4.0. |
|
@csr632 |
Posting this early for feedback / informative purposes.
I'm in the process of seeing if we can get depcache from previous versions of SystemJS specified for import maps directly here - WICG/import-maps#210.
Will be working off this experimental implementation to test things out further.