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 implementation #2134

Merged
merged 5 commits into from
Jul 23, 2020
Merged

depcache implementation #2134

merged 5 commits into from
Jul 23, 2020

Conversation

guybedford
Copy link
Member

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.

@guybedford
Copy link
Member Author

(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!)

Copy link
Collaborator

@joeldenning joeldenning left a 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.


const systemInstantiate = systemJSPrototype.instantiate;
systemJSPrototype.instantiate = function (url, firstParentUrl) {
const depcache = importMap.depcache[url];
Copy link

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?

Copy link
Collaborator

@joeldenning joeldenning Feb 28, 2020

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.

Copy link
Collaborator

@joeldenning joeldenning left a 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?

@guybedford
Copy link
Member Author

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.

@csr632
Copy link
Contributor

csr632 commented Mar 28, 2020

I can't wait to use this feature!
My use case:

  • top-app: depends on "react", dynamically load "external-lib"
  • external-lib: depends on "react"

external-lib is matained and deployed by another team

In order to share "react" between "top-app" and "external-lib", I am forced to build react as a separate systemjs bundle:

  • react.system.js
  • top-app.system.js
  • external-lib.system.js (deployed by another team)

Separating "react" lead to a loading waterfall:

  1. html and inline bootstrap code(systemjs runtime and System.import("top-app.system.js"))
  2. load top-app.system.js
  3. load react.system.js`. Top-app can‘t do anything before this finish!
  4. (when needed, dynamically load external-lib.system.js)

What I want is this:

  1. html and inline bootstrap code: systemjs runtime and System.import('top-app.system.js'); System.import('react.system.js');.
  2. load "top-app.system.js" and "react.system.js" in parallel. Top-app is functional after this finish
  3. (when needed, dynamically load external-lib.system.js)

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 ?

  1. html and inline bootstrap code: systemjs runtime and Inline "top-app.system.js" and "react.system.js". Top-app is functional after this finish
  2. (when needed, dynamically load external-lib.system.js)

I ask this question in another issue: #2148 (comment)

@guybedford
Copy link
Member Author

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?

@guybedford
Copy link
Member Author

(I'm going to check the numbers again on some larger projects with the goal to merge)

@joeldenning
Copy link
Collaborator

I don't have any problems with this implementation

@guybedford guybedford changed the title WIP: depcache implementation depcache implementation Jul 23, 2020
@github-actions
Copy link

github-actions bot commented Jul 23, 2020

core

File by file impact

File Transform Diff master depcache Event
dist/s.js none +639 21,190 21,829 changed
gzip +130 6,081 6,211
brotli +124 5,216 5,340
dist/s.min.js none +283 6,279 6,562 changed
gzip +86 2,515 2,601
brotli +81 2,258 2,339
dist/system-node.cjs none +224 200,761 200,985 changed
gzip +50 51,295 51,345
brotli +43 43,408 43,451
dist/system.js none +639 31,636 32,275 changed
gzip +127 8,584 8,711
brotli +128 7,413 7,541
dist/system.min.js none +283 10,304 10,587 changed
gzip +87 3,957 4,044
brotli +80 3,532 3,612
extras

File by file impact

Pull request have no impact on extras files.

Generated by github pull request filesize impact

@guybedford guybedford merged commit c8210d6 into master Jul 23, 2020
@guybedford guybedford deleted the depcache branch July 23, 2020 20:55
@guybedford
Copy link
Member Author

Released in 6.4.0.

@12kb
Copy link

12kb commented Oct 19, 2021

@csr632
What is about your issue? Isn't it possible to include react to top-app and use System.register('react', ...) there to provide it to external-lib? Looks like it worked for me.

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