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

prefetch dynamic modules when appcache is active #9482

Closed
wants to merge 5 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@CaptainN
Contributor

CaptainN commented Dec 18, 2017

This prefetches modules into the cache when the appcache package is detected.

As described in meteor/meteor-feature-requests#236

Couple of notes:

  • I coped the useful flattenModuleTree from client.js - I'm sure there's a better way to do that
  • I'm driving the initial preload off of window.load - I assume this is a good time to start it, as the initial application should already have been loaded, and started. I had it off of Meteor.startup at first, but window.load fires later - after images have loaded. If there's a better event, let me know.
// If we call module.prefetch(id) multiple times in the same tick of
// the event loop, all those modules will be fetched in one request.
function prefetchInChunks(modules, amount) {
var promises = Promise.all(modules.splice(0, amount).map(function (id) {

This comment has been minimized.

@zimme

zimme Dec 18, 2017

Contributor

var promises is unused, so I'm thinking you can just remove it?

// If Package.appcache is loaded, preload additional modules after
// the core bundle has been loaded
if (Package.appcache) {

This comment has been minimized.

@benjamn

benjamn Dec 19, 2017

Member

Let's move the check for Package.appcache inside the window.onload callback, just in case the appcache package loads after dynamic-import.

This comment has been minimized.

@benjamn

benjamn Dec 19, 2017

Member

Also, in order to make the window.onload event registration compatible with all browsers, I would do something like this:

if (global.addEventListener) {
  global.addEventListener("load", handler, false);
} else if (global.attachEvent) {
  global.attachEvent("onload", handler);
}

This comment has been minimized.

@CaptainN

CaptainN Dec 19, 2017

Contributor

I'll make these changes - what version of IE does Meteor try to maintain support for? Any thought to using something like http://github.com/Raynos/DOM-shim ? It would allow us to remove forking code like this and still allow users who need IE8 and lower a way forward.

Also (this is more for my education) - is global a meteor variable?

This comment has been minimized.

@CaptainN

CaptainN Dec 19, 2017

Contributor

Also, do you think it'd be worth exporting flattenModuleTree - and have client.js (which now includes a copy of that function) consume that? Alternatively, I could more specifically tailor the copy I added to dynamic-versions.js and just leave it copied (or just leave it as is).

This comment has been minimized.

@zimme

zimme Dec 19, 2017

Contributor

Also (this is more for my education) - is global a meteor variable?

https://github.com/tc39/proposal-global

This comment has been minimized.

@benjamn

benjamn Dec 19, 2017

Member

Yep, global is going to be standard ECMAScript eventually, and in the meantime the meteor package defines the global variable for all environments here.

I think this code is low-level enough that it's worth hand-coding cross-browser code, rather than bundling a library that we'd only use a part of. Likewise, I would support tailoring flattenModuleTree to the needs of this PR, especially if you see any opportunities to shorten/simplify the code.

This comment has been minimized.

@CaptainN

CaptainN Dec 19, 2017

Contributor

It's a minor change to flattenModuleTree - I could make it spit out the array I use directly, instead of converting that from the flat object it currently creates - it would use a bit less RAM, so I'll make that change.

})
}
// Get a flat array of modules and start prefetching.
prefetchInChunks(Object.keys(flattenModuleTree(versions)), 20);

This comment has been minimized.

@zimme

zimme Dec 19, 2017

Contributor

Should amount be configurable?

This comment has been minimized.

@benjamn

benjamn Dec 20, 2017

Member

That's a good question, but I'd like to wait and see if people need the configurability. That said, I think I am going to bump this up to 50.

if (global.addEventListener) {
global.addEventListener('load', precacheOnLoad, false);
} else if (global.attachEvent) {
global.attachEvent('load', precacheOnLoad);

This comment has been minimized.

@benjamn

benjamn Dec 20, 2017

Member

This should be onload instead of load (but only for global.attachEvent). Thanks, IE.

This comment has been minimized.

@CaptainN

CaptainN Dec 20, 2017

Contributor

Ooph! Good catch.

@benjamn benjamn added this to the Release 1.6.1 milestone Dec 20, 2017

benjamn added a commit that referenced this pull request Dec 20, 2017

@benjamn

This comment has been minimized.

Member

benjamn commented Dec 20, 2017

Merged to devel with commit 23e3ac9. Thanks for taking care of this, @CaptainN!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment