-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(service-worker): check that start_url is within SW's scope #6678
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't we just want to check that a service worker controls the start URL? i.e. if the one controlling this page is more specific but the start URL is the homepage and so it's controlled by a root service worker for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we may need to think about this a bit. I originally had that check, and I don't think there's any technical problem with it, but then was thinking of the issues if you're testing a page and the declared web app manifest creating the PWA containing that page had a
start_url
that was controlled by a different SW than the page...so I guess thestart_url
is part of a different PWA than the PWA the test page is part of?So then that seems to violate testing the page as part of a PWA. But also I don't know. But also this should only be possible when running with
--disable-storage-reset
anyways, so it's fine to be tougher or not tougher on them :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an example:
google.com/maps
has a service worker controlling everything under thegoogle.com/maps
scope. So if you go togoogle.com/maps
, you're served offline from that SW.However, if they accidentally put a
start_url
ofgoogle.com
in their manifest (andgoogle.com
has a SW), trying to load the maps PWA (e.g. from the homescreen) would load a completely different PWA than the one that you saved.Again, the likely Lighthouse user experience if someone ever accidentally does this is that storage was cleared and the
start_url
just doesn't work offline, but in the off chance they're running without clearing, I think it's better to fail on that case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK I buy that! SGTM