Fix for #560 leading slash added to files in manifest #589

happyahluwalia · 2017-05-26T23:30:55Z

Fixes #560

Description of what's changed/fixed.
Removed the leading slash being added to the files in the manifest for caching.
Also updated the test cases for the same.

Please ensure that gulp lint test passes locally prior to filing a PR!
Unable to test since I am on windows and selenium assistant works only on linux and OSX

[19:26:48] Starting 'mocha'...
C:\projects\github\workbox\node_modules\selenium-assistant\src\index.js:153
      throw new Error('Sorry this library only supports OS X and Linux.');
      ^

Error: Sorry this library only supports OS X and Linux.
    at SeleniumAssistant.getLocalBrowsers (C:\projects\github\workbox\node_modules\selenium-assistant\src\index.js:153:13)

googlebot · 2017-05-26T23:30:57Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

happyahluwalia · 2017-05-26T23:35:23Z

Signed it :-)

…

On Fri, May 26, 2017 at 7:30 PM, googlebot ***@***.***> wrote: Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 *Please visit https://cla.developers.google.com/ <https://cla.developers.google.com/> to sign.* Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks. ------------------------------ - If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. - If you signed the CLA as a corporation, please let us know the company's name. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#589 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHtPHgvWTvVjBlIiW-eHHEnqY2vE_krEks5r92CzgaJpZM4NoJpR> .

googlebot · 2017-05-26T23:35:25Z

CLAs look good, thanks!

addyosmani · 2017-05-27T00:01:05Z

In spirit, this change LGTM as reviewing the referenced thread @gauntface was okay with removing the forced forward slash. We're currently seeing failures on Windows (AppVeyor) here. I'll give the Travis build a kick as it seems to be taking longer than expected.

happyahluwalia · 2017-05-27T00:51:45Z

@addyosmani There is one test case Generate SW End-to-End Tests that is failing... not sure why,

Can you please help looking into this. I will take another look with fresh eyes again later..

…pdated the test case

gauntface · 2017-05-28T04:13:45Z

packages/workbox-build/test/node/get-file-manifest-entries.js

@@ -263,38 +263,38 @@ describe('Test getFileManifestEntries', function() {
      globDirectory: path.join(__dirname, '..', '..', '..',
        'workbox-cli', 'test', 'static', 'example-project-1'),
      dynamicUrlToDependencies: {
-        '/template/url1': ['page-1.html', 'index.html'],


Was this a required change or optional?

My gut says that both of these should be valid.

With the fix where all leading "/" are removed we need to ensure all the tests also do not point to root "/'. If the above tests are not updated then it errors out during the run....

for e.g. without the change to dynamicUrlToDependencies we get the following

7) Test Runner Environment Node Tests for workbox-build Test getFileManifestEntries should return file entries from example project with dynamicUrlToDependencies: AssertionError: expected [ Array(8) ] to deeply equal [ Array(8) ] + expected - actual [ { - "revision": "66f6a91fb924cf7e0b59d496a229c08a" + "revision": "24abd5daf6d87c25f40c2b74ee3fbe93" "url": "index.html" } { /* Deleted some stuff here to keep sample cleaner */ } { - "revision": "b7ccb766d7273d9c0095706bbce65a6e" - "url": "/template/url1" + "revision": "a505dfb0ac2cad8933ec437dd97ccc66" + "url": "template/url1" } { - "revision": "76d943bf4ee4fb547a7dd541464618ff" - "url": "/template/url2" + "revision": "bd9ef0ab8b57d5d716e6916610d34936" + "url": "template/url2" } { "revision": "538954a0f0fca1d067ff03dca8dce79e" - "url": "/template/url3" + "url": "template/url3" } ]

Ideally the developer could define with and without the forward slash for the templatedURL.

The only reason I say this is because if the service worker is nested under a specific directory, you may want the URL to have a slash in some scenarios and not in others so we should support both.

happyahluwalia · 2017-06-06T00:57:43Z

@gauntface can you please merge the conflicts, I dont have write access :)

gauntface · 2017-06-06T19:17:43Z

packages/workbox-build/test/node/get-file-manifest-entries.js

-        'template/url3': '<html><head></head><body><p>Just in case</p></body></html>',
+        '/template/url1': ['page-1.html', 'index.html'],
+        '/template/url2': ['page-2.html', 'index.html'],
+        '/template/url3': '<html><head></head><body><p>Just in case</p></body></html>',


Could you make the last one relative

gauntface · 2017-06-06T19:18:00Z

packages/workbox-build/test/node/get-file-manifest-entries.js

-        'template/url3': '<html><head></head><body><p>Just in case</p></body></html>',
+        '/template/url1': ['page-1.html', 'index.html'],
+        '/template/url2': ['page-2.html', 'index.html'],
+        '/template/url3': '<html><head></head><body><p>Just in case</p></body></html>',


Can you make the last one relative

@gauntface just so that I am clear - if we make the url3 (last one) relative wouldn't it give a problem when the html content it is referring to is updated

googlebot · 2017-06-06T19:18:36Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

gauntface · 2017-06-06T19:20:02Z

@jeffposnick Are you ok with this? I can't see any reason this should break anything, but wanted to confirm with you cos of precache experience.

jeffposnick · 2017-06-06T19:23:37Z

Definitely on board. Paths that are relative to the SW's location give more flexibility vs. forcing absolute paths at the root.

gauntface · 2017-06-09T21:16:41Z

@happyahluwalia sorry for the drag on this.

There is one last issue and it's this line here: https://github.com/happyahluwalia/workbox/blob/c712e16be26bfb0690d8bcbfd49422468b519f24/packages/workbox-cli/test/utils/validator/fake-run-and-compare.js#L22

Can you remove that forward slash and it should hopefully fix the remaining failing tests. I can't do it on your branch.

* Fix for #560 leading slash added to files in manifest (#589) * Fix for #560 leading slash added to files in manifest * Issue #560 end to end test updated to remove leading / * Issue #560 One test case also appended a leading slash, updated the test case * Preceeding slash is required in case of templateURL * Fix for #560 removing leading slash * Fixing up the remaining issues with leading slash change

Fix for GoogleChrome#560 leading slash added to files in manifest

6a3b87a

Issue GoogleChrome#560 end to end test updated to remove leading /

c136e15

Issue GoogleChrome#560 One test case also appended a leading slash, u…

d62c703

…pdated the test case

gauntface reviewed May 28, 2017

View reviewed changes

Preceeding slash is required in case of templateURL

dd41f55

gauntface reviewed Jun 6, 2017

View reviewed changes

Merge branch 'master' into master

1144910

Merge branch 'master' into master

c712e16

happyahluwalia added 3 commits June 9, 2017 18:26

Pulled and Merged from workbox master

0ba40a1

Mergin from upstream

2ccf047

Fix for GoogleChrome#560 removing leading slash

cf6ba35

gauntface changed the base branch from master to temp-leading-slash June 12, 2017 21:14

gauntface merged commit d5b96ca into GoogleChrome:temp-leading-slash Jun 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #560 leading slash added to files in manifest #589

Fix for #560 leading slash added to files in manifest #589

happyahluwalia commented May 26, 2017 •

edited

Loading

googlebot commented May 26, 2017

happyahluwalia commented May 26, 2017 via email

googlebot commented May 26, 2017

addyosmani commented May 27, 2017

happyahluwalia commented May 27, 2017 •

edited

Loading

gauntface May 28, 2017

happyahluwalia May 28, 2017

gauntface May 31, 2017

happyahluwalia commented Jun 6, 2017

gauntface Jun 6, 2017

gauntface Jun 6, 2017

happyahluwalia Jun 6, 2017

googlebot commented Jun 6, 2017

gauntface commented Jun 6, 2017

jeffposnick commented Jun 6, 2017

gauntface commented Jun 9, 2017

Fix for #560 leading slash added to files in manifest #589

Fix for #560 leading slash added to files in manifest #589

Conversation

happyahluwalia commented May 26, 2017 • edited Loading

googlebot commented May 26, 2017

happyahluwalia commented May 26, 2017 via email

googlebot commented May 26, 2017

addyosmani commented May 27, 2017

happyahluwalia commented May 27, 2017 • edited Loading

gauntface May 28, 2017

Choose a reason for hiding this comment

happyahluwalia May 28, 2017

Choose a reason for hiding this comment

gauntface May 31, 2017

Choose a reason for hiding this comment

happyahluwalia commented Jun 6, 2017

gauntface Jun 6, 2017

Choose a reason for hiding this comment

gauntface Jun 6, 2017

Choose a reason for hiding this comment

happyahluwalia Jun 6, 2017

Choose a reason for hiding this comment

googlebot commented Jun 6, 2017

gauntface commented Jun 6, 2017

jeffposnick commented Jun 6, 2017

gauntface commented Jun 9, 2017

happyahluwalia commented May 26, 2017 •

edited

Loading

happyahluwalia commented May 27, 2017 •

edited

Loading