Skip to content
This repository has been archived by the owner on Aug 10, 2022. It is now read-only.

Work around "Update on Reload" infinite loop bug #5515

Closed
wants to merge 1 commit into from

Conversation

dfabulich
Copy link
Contributor

When you check "Update on Reload" in Chrome Dev Tools and you configure the page to reload on every controller change, it refreshes the page in an infinite loop. We can work around this by using a variable to refresh the page only once.

Target Live Date: YYYY-MM-DD

  • This has been reviewed and approved by (NAME)
  • I have run gulp test locally and all tests pass.
  • I have added the appropriate type-something label.
  • I've staged the site and manually verified that my content displays correctly.

R: @petele

When you check "Update on Reload" in Chrome Dev Tools and you configure the page to reload on every controller change, it refreshes the page in an infinite loop. We can work around this by using a variable to refresh the page only once.
@dfabulich
Copy link
Contributor Author

@dfabulich
Copy link
Contributor Author

@gauntface will you be available to review this soon?

@gauntface
Copy link

Sorry I thought I already commented on this.

If there are multiple tabs - this will only update the one page the user was currently on I.e. background tabs won't be updated, this was the main reason the messaging I was using before. Most developers seem to want that behavior of refreshing all pages, so I'm not sure we should recommend this just to get around a DevTools quirk. What do you think?

@gauntface
Copy link

It's also worth noting that this behaviour in Chrome Canary seems to have changes to no longer skip waiting and claim clients.

@dfabulich
Copy link
Contributor Author

I think you might be thinking of a different PR. This PR just adds a local refreshing variable to prevent each tab from double-reloading. It should not have any different effect on background tabs.

I have a sample repository https://github.com/dfabulich/service-worker-refresh-sample that copies and pastes from the Advanced recipe guide; I just pushed a dev-tools-workaround branch incorporating the proposed fix in this PR.

Here's what I did:

  1. npm install && npm run start
  2. Opened http://localhost:8000 in an incognito window
  3. Refreshed to make sure the client is claimed
  4. Opened a second tab to http://localhost:8000
  5. Edited index.html to replace the "0" in the body tag on line 2 with a "1", and edited index.html to change the first line v variable from "0" to "1".
  6. Refreshed one browser tab. The reload prompt button appeared.
  7. I clicked the button. The page refreshed and I saw a "1" on the screen.
  8. I switched back to the other tab. That tab had also refreshed, and was now also showing a "1", as expected.

Are you able to find a multiple-tab bug in my sample repro? If so, can you tell me how to reproduce that bug?

@dfabulich
Copy link
Contributor Author

dfabulich commented Dec 18, 2017

I should point out that step 3 in that list is subtle and important, because my sample doesn't call clients.claim() during activation.

So if you skip step 3 in the first tab, the only controlled client will be the second tab; in that case, if you click the button in step 7, and then switch back to the first tab in step 8, well, it never had a controller and still doesn't, so it doesn't refresh.

That's true regardless in both branches of my sample, before and after the change proposed in this PR.

EDIT: It turns out that if the SW does clients.claim() during activation, that causes issue #5535; I've put together a PR #5536 to fix that.

@petele
Copy link
Member

petele commented Dec 19, 2017

@gauntface - merge if/when you're happy.

@gauntface
Copy link

@dfabulich closing this in favor of https://github.com/google/WebFundamentals/pull/5536/files which has the same changes.

@gauntface gauntface closed this Dec 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants