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

0.18.0 breaks registerType: "prompt" #663

Closed
alexlapwood opened this issue Feb 16, 2024 · 13 comments · Fixed by #673
Closed

0.18.0 breaks registerType: "prompt" #663

alexlapwood opened this issue Feb 16, 2024 · 13 comments · Fixed by #673
Labels
bug Something isn't working

Comments

@alexlapwood
Copy link

alexlapwood commented Feb 16, 2024

Expected behaviour

In 0.17.5 setting registerType: "prompt" behaved like this:

  1. Check for updates with serviceWorkerRegistration.update()
  2. If a new update is found show a prompt
  3. When the prompt is activated reload the page to active the new service worker with updateServiceWorker()

Observed behaviour

In 0.18.0 setting registerType: "prompt" behaves like this:

  1. Check for updates with serviceWorkerRegistration.update()
  2. If a new update is found the page automatically reloads (this is the bug)
  3. Curiously the new service worker still needs to be activated with updateServiceWorker() (causing another reload)

Steps to reproduce

You can reproduce this issue using the following SolidJS hook:

export function useUpdate() {
  const [serviceWorkerRegistration, setServiceWorkerRegistration] =
    createSignal<ServiceWorkerRegistration | undefined>(undefined);

  const {
    needRefresh: [needRefresh],
    updateServiceWorker,
  } = useRegisterSW({
    onRegistered(serviceWorkerRegistration) {
      setServiceWorkerRegistration(serviceWorkerRegistration);
      setInterval(() => {
        serviceWorkerRegistration?.update();
      }, 60 * 1000);
    },
    onRegisterError(error) {
      console.error("SW registration error", error);
    },
  });

  return { updateReady: needRefresh, updateServiceWorker };
}

I'm not sure if the title of this issue is correct, please change as needed.

@userquin
Copy link
Member

userquin commented Feb 16, 2024

Why are you exposing update instead updateServiceWorker? I need to check if calling await serviceWorkerRegistration()?.update(); will skip the awaiting state (this call seems to be the problem).

In the meantime, can you test using the logic provided in the periodic sync for updates and exposing updateServiceWorker in the update value?

@alexlapwood
Copy link
Author

alexlapwood commented Feb 16, 2024

Why are you exposing update instead updateServiceWorker?

Good shout, this is completely redundant.

In the meantime, can you test using the logic provided in the periodic sync for updates and exposing updateServiceWorker in the update value?

I'm seeing the same issue with this logic, here is the updated hook if you'd like to double check my changes:

export function useUpdate() {
  const {
    needRefresh: [needRefresh],
    updateServiceWorker,
  } = useRegisterSW({
    onRegisteredSW(serviceWorkerUrl, serviceWorkerRegistration) {
      setInterval(async () => {
        if (!(!serviceWorkerRegistration?.installing && navigator)) {
          return;
        }

        if ("connection" in navigator && !navigator.onLine) {
          return;
        }

        const response = await fetch(serviceWorkerUrl, {
          cache: "no-store",
          headers: {
            cache: "no-store",
            "cache-control": "no-cache",
          },
        });

        if (response?.status === 200) {
          await serviceWorkerRegistration?.update();
        }
      }, 60 * 1000);
    },
    onRegisterError(error) {
      console.error("SW registration error", error);
    },
  });

  return { updateReady: needRefresh, updateServiceWorker };
}

@userquin
Copy link
Member

userquin commented Feb 16, 2024

Can you try removing the interval in your local? I Will check your reproduction tmr (refresh the page once built second time).

@alexlapwood
Copy link
Author

alexlapwood commented Feb 17, 2024

Can you try removing the interval in your local? I Will check your reproduction tmr (refresh the page once built second time).

Removing setInterval and refreshing the page works as expected, the update installs and it doesn't trigger a refresh.

I did a separate test to see what happens if we update the service worker with Chrome's dev tools (also with setInterval removed) and it behaves the same way setInterval does. Once the update is waiting to activate, the page refreshes itself (but still waits for the user to trigger updateServiceWorker , causing a second refresh).

@userquin
Copy link
Member

You Will need to avoid calling update if the sw also waiting to be activated, I guess the update will also trigger the event to active it

@userquin
Copy link
Member

userquin commented Feb 17, 2024

It seems the problem is the heuristic time approach in worbox window to detect false positives: you can try awaiting 1 minute between builds (https://github.com/GoogleChrome/workbox/blob/v7/packages/workbox-window/src/Workbox.ts#L414)

@alexlapwood
Copy link
Author

Thank you for all your help, looking forward to getting to the bottom of this.

You Will need to avoid calling update if the sw also waiting to be activated

This doesn't seem to help, even if I remove serviceWorkerRegistration?.update() the issue still happens when I manually trigger the update from the Chrome dev tools (Chrome isn't aware of a new service worker at all at this point).

It seems the problem is the heuristic time approach in worbox window to detect false positives: you can try awaiting 1 minute between builds (https://github.com/GoogleChrome/workbox/blob/v7/packages/workbox-window/src/Workbox.ts#L414)

I tried the following but the issue still occurs:

  1. 1st build
  2. Install service worker and refresh
  3. Wait 10 minutes
  4. Second build
  5. Manually trigger service worker update from Chrome dev tools
  6. Service worker installing...
  7. Service worker waiting -> automatic refresh (bug)
  8. After refresh needRefresh is true and service worker is still waiting to be activated (expected)

@userquin
Copy link
Member

userquin commented Feb 17, 2024

Cleanup the storage removing the sw, the logic is always 1 step behind, you don't need to wait 10 min. just 1 min.

EDIT: I have no idea what's the behavior when using refresh from chrome dev tools, you can just await the periodic sync to refresh it or just refresh the page

@alexlapwood
Copy link
Author

I have no idea what's the behavior when using refresh from chrome dev tools

I think it's the same as calling serviceWorkerRegistration.update() once.

you can just await the periodic sync to refresh it or just refresh the page

Sorry I'm not sure what you mean by this.

Cleanup the storage removing the sw

Sorry I'm not sure what you mean by this either.

@userquin
Copy link
Member

I'm going to revert last change, I need to review what's happening in workbox-window and the register module here, sw events seems to be wrong.

@userquin userquin added the bug Something isn't working label Feb 20, 2024
@userquin
Copy link
Member

I guess it is a problem with the heuristic time based and short sw periodic sync + update call (when precaching new sw and calling update)

@userquin
Copy link
Member

userquin commented Feb 20, 2024

@alexlapwood can you try this approach (update signals and effect, I'm not a solid expert)?

  const [updateFound, setUpdateFound] = createSignal(false)
  const [interval, registerInterval] = createSignal<ReturnType<typeof setInterval> | undefined>(undefined)

  createEffect(() => {
    if (updateFound())
      clearInterval(interval())
  })

then in your onRegisteredSW callback:

onRegisteredSW(swUrl, r) {
	if (r) {
	  navigator.serviceWorker.ready.then((registration) => {
		registration.addEventListener('updatefound', () => {
		  console.log('updatefound')
		  setUpdateFound(true)
		})
	  })
	  registerInterval(setInterval(async () => {
		if (!(!r.installing && navigator))
		  return

		if (('connection' in navigator) && !navigator.onLine)
		  return

		const resp = await fetch(swUrl, {
		  cache: 'no-store',
		  headers: {
			'cache': 'no-store',
			'cache-control': 'no-cache',
		  },
		})

		if (resp?.status === 200)
		  await r.update()
	  }, 20000))
	}
}

@alexlapwood
Copy link
Author

can you try this approach (update signals and effect, I'm not a solid expert)?

Your use of signals and effects looks good to me, but the same issue still occurs. Let me know if there's anything else I can do to help figure this one out.

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.

2 participants