Skip to content

feat: add offline support #80

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

Merged
merged 27 commits into from
Jun 9, 2020

Conversation

Siemko
Copy link
Contributor

@Siemko Siemko commented Jun 1, 2020

Hi there, so basically I've seen #29 and thought of giving it a try. I used some basic service worker to cache everything on / route. Let me know if this is enough.

@Siemko Siemko changed the title feat: turn app into PWA WIP: feat: turn app into PWA Jun 1, 2020
@Siemko
Copy link
Contributor Author

Siemko commented Jun 1, 2020

All right, so the biggest problem with that is to wait for parcel to bundle the app and then generate service worker that will precache all the files. I will try to make it work.

@Siemko
Copy link
Contributor Author

Siemko commented Jun 1, 2020

Ok, so client files in main directory are cached properly. I have to work out how to make it work with Netlify redirects to cache all the files for offline usage, because currently the state of PWA is that it doesn't serve the index.html from cache. I'll have a look at it later.

@Siemko
Copy link
Contributor Author

Siemko commented Jun 1, 2020

Alright, so basically I've got it working. It needs some polishing (manifest icons are not loaded) and I've got one problem: URL without markup query works offline, but with markup query seems broken.

@Siemko
Copy link
Contributor Author

Siemko commented Jun 1, 2020

I can happily confirm that offline veriosn is working properly.

@Siemko Siemko changed the title WIP: feat: turn app into PWA feat: turn app into PWA Jun 1, 2020
@Siemko
Copy link
Contributor Author

Siemko commented Jun 2, 2020

@smeijer can you have a look at that? I guess if it's not on the roadmap, it's less important, but I still did it.

@smeijer
Copy link
Member

smeijer commented Jun 2, 2020

I'll check this tomorrow. As it's about bed time in my timezone.

I need to read into workbox, as I've never heard of it before.

And a quick look also makes me wonder if the chunks of javascript really need to be in those html files.

But that's just a quick 2 minute glimpse at the diff. I'll come back to you with a more complete response.

Again, thanks for contributing. Offline functionality wasn't added to roadmap.md, but it is definitely something great to have. Hence the issue.

@Siemko
Copy link
Contributor Author

Siemko commented Jun 3, 2020

And a quick look also makes me wonder if the chunks of javascript really need to be in those html files.

The only part necessary is:

const sw = '/sw.js';
navigator.serviceWorker.register(sw)

the line with const is a trick to make parcel work with workbox-builder.

@smeijer
Copy link
Member

smeijer commented Jun 5, 2020

@Siemko, sorry it took me so long.

I have a couple of questions/suggestions;

  1. I think we should move the js code out of index.html into a service-worker.js. Just to clean it up.

  2. Regarding this message:

    console.log(
      'New content is available and will be used when all ' +
      'tabs for this page are closed. See https://bit.ly/CRA-PWA.',
    );
    

    Where does it come from? We're not using CRA? Can we also change that to a fancy "there is an update waiting, click here to reload" button?

  3. When running npm run start, I'll get an error because there is no sw.js.

@smeijer smeijer marked this pull request as draft June 5, 2020 14:25
@Siemko
Copy link
Contributor Author

Siemko commented Jun 5, 2020

  1. I think we should move the js code out of index.html into a service-worker.js. Just to clean it up.

Agreed, I also included production env checking.

  1. Regarding this message:
    console.log(
      'New content is available and will be used when all ' +
      'tabs for this page are closed. See https://bit.ly/CRA-PWA.',
    );
    
    Where does it come from? We're not using CRA? Can we also change that to a fancy "there is an update waiting, click here to reload" button?

I have taken it out from CRA. :) Sure will add the fancy button or any other message.

  1. When running npm run start, I'll get an error because there is no sw.js.

Yes, I forgot to include env checking.

Still working on that fancy alert/button thing. Can you tell me, how should it look?

@smeijer
Copy link
Member

smeijer commented Jun 6, 2020

Can you tell me, how should it look?

This is how Google does it, what do you think?

image

Maybe we can use react-toastify for it? That would also allow us to show a toast for other actions (like copy to clipboard) later.

@Siemko
Copy link
Contributor Author

Siemko commented Jun 7, 2020

@smeijer sorry for the late response, I gave it a shot, I put react-toastify there as suggested.

@smeijer smeijer marked this pull request as ready for review June 9, 2020 13:34
@smeijer
Copy link
Member

smeijer commented Jun 9, 2020

@Siemko, I think it's looking great!

Few things though,

  1. Can you provide me with instructions on how to test the "update available" toast? How do I trigger that "update"? 😇

  2. Can we change the update message?

    - An update is ready!
    + A new version is available!
  3. It only works for the root domain, and that's an issue. When the user goes to "https://testing-playground.com" it works, but when he is hitting refresh while he is editing a playground, it still results in ERR_INTERNET_DISCONNECTED. This is because the playground adds state in the query params: .com?markup=...

    Will it be possible to fix/support that?

I can't wait to have this branch merged. Great work! 😎

@Siemko
Copy link
Contributor Author

Siemko commented Jun 9, 2020

@smeijer, thank you :)

  1. Can you provide me with instructions on how to test the "update available" toast? How do I trigger that "update"? innocent

Uploading new version of built files (so, basically files with new hashes) will trigger the SW to update. It is based on "manifest", that is generated on each build.

So, basically it's available now!
https://i.imgur.com/l1Pixsl.png

  1. Can we change the update message?
    - An update is ready!
    + A new version is available!

Done.

  1. It only works for the root domain, and that's an issue. When the user goes to "https://testing-playground.com" it works, but when he is hitting refresh while he is editing a playground, it still results in ERR_INTERNET_DISCONNECTED. This is because the playground adds state in the query params: .com?markup=...
    Will it be possible to fix/support that?

I've solved that issue before, here's little PoC:

https://i.imgur.com/S35CWZl.gif

I can't wait to have this branch merged. Great work!

Me too 👍

@smeijer smeijer changed the title feat: turn app into PWA feat: add offline support Jun 9, 2020
@smeijer smeijer merged commit d91f655 into testing-library:develop Jun 9, 2020
@smeijer
Copy link
Member

smeijer commented Jun 9, 2020

@Siemko thanks for this one! Awesome work.

I've:

  • fixed a merge conflict in package.json (which I fucked up)
  • removed a console.log statement
  • added description, lang and orientation to the manifest
  • fixed my fuckup

I hope you don't mind 😇

Again, thanks. This is really nice.

@Siemko
Copy link
Contributor Author

Siemko commented Jun 9, 2020

Awesome, thank you @smeijer. Glad you put it up into work :)

@smeijer
Copy link
Member

smeijer commented Jun 9, 2020

@all-contributors please add @Siemko for code

@allcontributors
Copy link
Contributor

@smeijer

I've put up a pull request to add @Siemko! 🎉

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.

2 participants