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

Add stub service worker so users can install on desktop with Chrome #11774

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

daleharvey
Copy link
Contributor

@daleharvey daleharvey commented Jan 4, 2020

As discussed in #11646


Fixes #11646

@daleharvey
Copy link
Contributor Author

So the sw.js is just a stub file, it needs a fetch event listener or chrome will not consider it installable.

We could of course actually cache all the static assets, it would have a far bigger impact on performance than any of the reducing bundle size type optimisations, however that seemed like a lot of work given that its been mentioned riot may not always support the web (#11646 (comment))

At the least this makes it installable and adding the actual caching could be done as a follow up

src/vector/index.html Outdated Show resolved Hide resolved
@jryans jryans requested a review from a team January 6, 2020 13:28
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

@daleharvey
Copy link
Contributor Author

daleharvey commented Jan 7, 2020

Updated the script tag to move it to line 44 as suggested, also here is what it looks like installed

Screenshot 2020-01-07 11 14 55

@turt2live
Copy link
Member

@daleharvey , @jryans will be taking this one on.

@turt2live turt2live dismissed their stale review January 9, 2020 04:54

dismissing because it's not really relevant

Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @daleharvey! I think this looks good technically, and we're going to give this a try in an experimental capacity, under the assumption that installable PWAs should just work the same as the regular web app.

As one last step, could add your sign off to the PR as described in CONTRIBUTING.rst? Once you've done that, we should be ready to merge. You can just add a comment with the sign off line if you like.

@jryans
Copy link
Collaborator

jryans commented Jan 10, 2020

its been mentioned riot may not always support the web (#11646 (comment))

Just to clarify for others, Riot is committed to supporting both web and desktop for the long term. See #11646 (comment) for more detail.

Signed-off-by: Dale Harvey <dale@arandomurl.com>
@daleharvey
Copy link
Contributor Author

Awesome thanks, I added the Signed-off to the commit

@jryans jryans merged commit 0e65d2e into element-hq:develop Jan 13, 2020
@jryans
Copy link
Collaborator

jryans commented Jan 13, 2020

Thanks for working on this! 😁 It should show up on https://riot.im/develop in a few minutes and in the next release in a few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Progressive Web App (PWA)
4 participants