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.

Sign up for GitHub

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

Fix Service Worker matching external top-level URLs #3191

Merged
merged 6 commits into from
Jun 11, 2021
Merged

Fix Service Worker matching external top-level URLs #3191

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
efe28fe
Fix Service Worker matching external top-level URLs
millennialdeveloper May 25, 2021
565b8ac
Prettify commit efe28fe7d17e800175e6891672e9af4e5cf5cc55
millennialdeveloper May 25, 2021
d7fb4cd
Merge branch 'develop' into patch-1
tjwiebell Jun 9, 2021
7e575c3
Add test for change
tjwiebell Jun 10, 2021
684cd43
Merge branch 'develop' into patch-1
tjwiebell Jun 10, 2021
7895d27
Merge branch 'develop' into patch-1
tjwiebell Jun 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/venia-concept/src/ServiceWorker/registerRoutes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export default function() {
* and the cycle repeats.
*/
registerRoute(
({ url }) => isHTMLRoute(url),
({ url }) => url.origin === self.location.origin && isHTMLRoute(url),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? The SW only has access to routes under the scope, in this case, the domain that launched it.

Copy link
Contributor Author

@millennialdeveloper millennialdeveloper May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about the whole service worker implementation in pwa-studio, but from here it seems that workbox may intercept external resources.

And without this change the script was loading through the service worker. The network tab showing it with a gear icon, and the response being the index template.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? The SW only has access to routes under the scope, in this case, the domain that launched it.

@revanth0212 I think, based on the link provided, if you pass a regex it will only match against the sw domain, but in our case we are intercepting all urls in the callback function.

Additionally, based on the isHomeRoute function it seems like ANY url without sub-paths ie www.google.com or zopim.com would return true so the SW would return our index. And if there was a sub path that happened to match the store code like www.google.com/en_us it would also return true and therefor our index file.

So if an external asset (like zen desk's js) is fetched from root, or if a file is requested from a path that is the first level that matches our store code, it will hit this sw route.

    if (url.pathname === '/') {
        return true;
    }

    // If store code is in the url, the home route will be url.com/view_code.
    // A trailing / may or may not follow.
    if (process.env.USE_STORE_CODE_IN_URL === 'true') {
        return AVAILABLE_STORE_VIEWS.some(
            ({ code }) =>
                url.pathname === `/${code}/` || url.pathname === `/${code}`
        );
    }
};```

new StaleWhileRevalidate({
plugins: [
{
Expand Down