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

Can the Event Listener for 'online' which triggers a reload be configurable? #232

Closed
g-farrow opened this issue Jul 6, 2021 · 4 comments · Fixed by #236
Closed

Can the Event Listener for 'online' which triggers a reload be configurable? #232

g-farrow opened this issue Jul 6, 2021 · 4 comments · Fixed by #236
Labels
bug Something isn't working

Comments

@g-farrow
Copy link
Contributor

g-farrow commented Jul 6, 2021

Summary

The behaviour of next-pwa which causes the page to be reloaded when the browser goes back "online" is not always the desired behaviour. When this happens it causes the app to lose its most recent state and interrupts the Users experience.

The code which causes this is here:

next-pwa/register.js

Lines 77 to 79 in e1d3129

window.addEventListener('online', () => {
location.reload()
})

Is there a reason why this code exists (i.e. is it solving another problem)? If it was to be removed/disable would it cause other unexpected behaviour or defects?

Could this behaviour be configurable?

Versions

  • next-pwa: 5.2.21
  • next: 11.0.0

How To Reproduce

Steps to reproduce the behavior:

  1. Go to a next-pwa app
  2. Switch the network to "offline" in Dev Tools
  3. Switch the network to "online" in Dev Tools
  4. See the page reload

Expected Behaviors

Ideally, we can set a new flag in next.config.js e.g.:

module.exports = withPWA({
  pwa: {
    dest: 'public',
    swSrc: '/src/service-worker/index.js',
    cacheOnFrontEndNav: true,
    reloadOnOnline: false
  }
})

Additional Context

If this sounds reasonable, I'm happy to open a PR 👍

@g-farrow g-farrow added the bug Something isn't working label Jul 6, 2021
@shadowwalker
Copy link
Owner

Go ahead for the PR

@g-farrow
Copy link
Contributor Author

Thanks @shadowwalker - I've created PR #236

@devfaic
Copy link

devfaic commented Jul 14, 2021

@shadowwalker Waiting for the new version with this change included. Thanks @g-farrow for the solution.

@shadowwalker
Copy link
Owner

Published in 5.2.24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants