-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 implementation #2134
depcache implementation #2134
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!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.