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

Preload dynamic packages when appcache is active #236

Closed
CaptainN opened this issue Dec 12, 2017 · 5 comments
Closed

Preload dynamic packages when appcache is active #236

CaptainN opened this issue Dec 12, 2017 · 5 comments

Comments

@CaptainN
Copy link

@CaptainN CaptainN commented Dec 12, 2017

Migrating from meteor/meteor#9407

When appcache is enabled, and dynamic-imports are used, Meteor is not able to load dynamic resources when loading from the cache, if those resources have not been previously loaded.

Solution - when appcache is enabled, dynamic-imports should preload all additional packages. A package manifest is already available, we just need a loop to grab them at some point after the initial bundle is loaded and run.

I'm happy to take a stab at a PR. It seems the changes could go in either the appcache package (detect if dynamic-imports is used) or in dynamic-imports (detect if appcache is used). Any preference?

@benjamn
Copy link
Member

@benjamn benjamn commented Dec 13, 2017

I would put it in dynamic-import.

Specifically, in dynamic-versions.js, you could add some code that walks the versions tree and calls module.preload(id) with each module identifier. The fetched modules will be added to the IndexedDB cache automatically.

It might make sense to spread this prefetching out over time, asynchronously. If you call module.preload(id) multiple times in the same tick of the event loop, all those modules will be fetched in one request. So you can chunk the requests to avoid making lots of individual requests. Note that module.preload(id) returns a Promise that resolves when the code is available to be imported, without evaluating the module.

You can test if appcache is used by checking if Package.appcache is a truthy object.

@tophsic
Copy link

@tophsic tophsic commented Mar 30, 2018

Hi,

I'm looking into using appcache package on an app using already dynamic imports. I thought @CaptainN work on this PR and meteor/meteor#9482 PR was to cache dynamic imported modules into Application Cache so they will be available offline. But I can not build a tiny simple app this way (https://github.com/tophsic/meteor-bare/tree/appcache-with-dynamic-imports).

When I load app online, I do not see dynamic.js file loaded into Application Cache and got this error if I switch to offline.

capture du 2018-03-30 16-35-58

Did I misunderstood object of this feature request? Is it possible to load dynamic import on offline mode?

Best regards

@benjamn
Copy link
Member

@benjamn benjamn commented Mar 30, 2018

Are you running the app with meteor --production? The IndexedDB caching of dynamic modules is only enabled in production, because caching tends to create confusion in development.

I suppose we could enable caching in development if you're using the appcache package, though it would be nice to have a way to turn it off.

@tophsic
Copy link

@tophsic tophsic commented Mar 30, 2018

Ouch, I think it is not the first time I forgot to think about such subtlety! Thanks for your extremelly quick response.

As far I am aware of this, I do not need caching in development environment.

@CaptainN
Copy link
Author

@CaptainN CaptainN commented Mar 30, 2018

I think it does run in development mode (at least it does print some info to the console), though you are likely to run over the generally small 5MB limit.

I can confirm that the PR I submitted did work when I submitted it, though I have seen some issues with it recently. I'll look at it more closely (with Meteor 1.6.1), since it's on my short list for a particular project I'm on anyway. :-)

This and the more recent web.browser.legacy stuff makes me thing we could use some unit tests around this - but I'm not sure how to set those up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants