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

remove dynamic js and map files from appcache (app.manifest) #9434

Merged
merged 2 commits into from Dec 5, 2017

Conversation

Projects
None yet
4 participants
@CaptainN
Contributor

CaptainN commented Dec 4, 2017

As described in issue #9407, dynamic-imports does not load modules from the appcache, and since we don't want the browser to download these assets twice, this patch avoids adding those files to app.manifest.

This patch does pass the appcache unit test, but I'm not sure how to create a test for this, as I'm not sure how to set gt it to load dynamic imports to test with.

remove dynamic js and map files from appcache (app.manifest) since th…
…ey aren't loaded from there by dynamic-imports
@meteor-bot

This comment has been minimized.

meteor-bot commented Dec 4, 2017

@CaptainN: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@hwillson

Thanks for submitting this PR @CaptainN! I'm sure @benjamn will have functionality comments, but I've mentioned a few small nit picky code formatting details. We try to make sure new code follows the same formatting as the existing source file (as much as possible). Thanks again!

@@ -39,6 +39,13 @@ var browserDisabled = function (request) {
return disabledBrowsers[request.browser.name];
};
function isDynamic (resource) {

This comment has been minimized.

@hwillson

hwillson Dec 4, 2017

Member

Since this is a named function, please remove the space between the function name and the parens: function isDynamic(resource) {.

@@ -97,7 +104,8 @@ WebApp.connectHandlers.use(function (req, res, next) {
manifest += "/" + "\n";
_.each(WebApp.clientPrograms[WebApp.defaultArch].manifest, function (resource) {
if (resource.where === 'client' &&
! RoutePolicy.classify(resource.url)) {
! RoutePolicy.classify(resource.url) &&
!isDynamic(resource)) {

This comment has been minimized.

@hwillson

hwillson Dec 4, 2017

Member

Please adjust your logical not's to have a space after the !.

@benjamn

benjamn approved these changes Dec 5, 2017

I appreciate that this PR involves changes only to the appcache package, rather than changing the way we bundle and serve JS resources (which would require a new version of Meteor to be released). Thanks for spotting this problem and moving forward with the fix, @CaptainN!

function isDynamic(resource) {
return resource.type === 'dynamic js' ||
(resource.type === 'json' &&
resource.url.startsWith('/dynamic/') &&

This comment has been minimized.

@benjamn

benjamn Dec 5, 2017

Member

This URL prefix may change after #9439, which makes these URLs look like /__browser/dynamic/... but that's OK—this is mostly a note to myself to update this code in that PR.

@benjamn benjamn added this to the Package Patches milestone Dec 5, 2017

@benjamn benjamn merged commit 491f678 into meteor:devel Dec 5, 2017

12 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Group 7 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjamn

This comment has been minimized.

Member

benjamn commented Dec 5, 2017

@CaptainN These changes have been published in appcache@1.1.1. Please let us know if you encounter any further problems after running meteor update appcache.

@CaptainN

This comment has been minimized.

Contributor

CaptainN commented Dec 5, 2017

Awesome! Thanks!

The url check is really only used for .map files. The check for dynamic js files was relatively easier, since the type attribute reflects it's dynamic status. If the .map files were similarly marked as dynamic in the type (like 'dynamic json') that might help avoid the problem.

Alternatively, we could remove all .map files from the app cache, and not inspect the url at all, but that's a bigger change.

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