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.

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

Default to SW mode when accessing app as PWA #809

Merged

Conversation

Jaifroid
Copy link
Member

@Jaifroid Jaifroid commented Jan 29, 2022

Fixes #808 and #801. Basically a one-liner, but formatted as two for legibility 😊.
EDIT: It will be interesting to see what effect this has on the secure browser tests...

@Jaifroid Jaifroid added this to the v3.3 milestone Jan 29, 2022
@Jaifroid Jaifroid self-assigned this Jan 29, 2022
@Jaifroid
Copy link
Member Author

Interesting that all tests passed. I was expecting the secure browser tests to fail due to differences between jQuery and SW mode, but clearly the differences are immaterial. That's good.

@Jaifroid Jaifroid requested a review from mossroy January 29, 2022 11:06
Copy link
Contributor

@mossroy mossroy left a comment

Choose a reason for hiding this comment

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

Good idea, even if directly accessing the PWA is not the main usage

www/js/app.js Outdated Show resolved Hide resolved
@mossroy
Copy link
Contributor

mossroy commented Jan 30, 2022

Interesting that all tests passed. I was expecting the secure browser tests to fail due to differences between jQuery and SW mode, but clearly the differences are immaterial. That's good.

That's very interesting.
Do we have a way to check which mode has been used by each browser from the UI tests logs?
Based on https://caniuse.com/mdn-api_serviceworker, it should use jQuery mode for Edge 15, and SW mode for all other ones (as IE11 does not run these tests at all)

@Jaifroid
Copy link
Member Author

That's very interesting. Do we have a way to check which mode has been used by each browser from the UI tests logs? Based on https://caniuse.com/mdn-api_serviceworker, it should use jQuery mode for Edge 15, and SW mode for all other ones (as IE11 does not run these tests at all)

I think it might depend on whether we can see the console output in the tests snapshots. If they are running in a secure context and the browser is capable of supporting SW mode, they should launch in SW mode with this PR.

@Jaifroid
Copy link
Member Author

Jaifroid commented Jan 31, 2022

So I guess this is ready to merge. I've tested on Firefox & Edge Chromium from localhost. You have to reset the app to see what it defaults to when there is no contentinjectionmode key in the localStorage. In both browsers it defaults to SW mode. In IE11 of course it remains in jQuery mode after a reset (what I mean is, it doesn't attempt and fail to load SW mode).

Copy link
Contributor

@mossroy mossroy left a comment

Choose a reason for hiding this comment

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

OK for me

@Jaifroid Jaifroid merged commit 0a78496 into master Jan 31, 2022
@Jaifroid Jaifroid deleted the Default-to-serviceworker-mode-when-accessing-app-as-PWA branch January 31, 2022 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Service Worker the default if the app is being accessed as a PWA
2 participants