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

Service worker #75

Merged
merged 10 commits into from
Nov 29, 2022
Merged

Service worker #75

merged 10 commits into from
Nov 29, 2022

Conversation

psrpinto
Copy link
Member

@psrpinto psrpinto commented Oct 19, 2022

Fixes #70

This PR introduces the Service Worker from hydrogen, which has two responsibilities:

  1. Provide offline functionality so that the client is usable when there is no network connection.
    • This means that assets (*.html, *.css, etc) are cached by the service worker so it's possible to load the client when there is no internet connection.
    • This is probably not super-useful for our use case, because if the main page (i.e. the parent of the iframe) does not itself have a service worker, the iframe will never load, nor will its service worker.
  2. Prevent concurrent access to the matrix store.
    • Fixes Issues when app is open in multiple tabs #70
    • Store might get corrupted without this.
    • Concurrent access happens when there's more than one service worker client (i.e. two tabs in the same browser instance, or two instances of chatrix on the same page, or a combination of both).
    • At any single moment, there is only one instance that has access to the store (Local storage).
    • Whenever a (non-active) instance tries to read/write from/to the store, the session will be closed in the active instance. Closing the session means the user is sent to the session selection screen.
    • So it's not possible to have more than one instance active at the same time.

TODO

Screen captures

Screenshot 2022-11-28 at 18 03 26

Screen.Recording.2022-11-29.at.11.59.15.mov

@psrpinto psrpinto linked an issue Oct 19, 2022 that may be closed by this pull request
@psrpinto psrpinto force-pushed the service-worker branch 2 times, most recently from efb72fd to 65c434d Compare October 20, 2022 14:50
@psrpinto
Copy link
Member Author

There is currently an issue when updating the service worker: when a new version of the service worker exists, refreshing the page causes all subsequent fetch requests to fail.

This doesn't happen in hydrogen, which makes me think that it might be related to the fact that we're running in an iframe. I've invested significant time into debugging this without success.

For the sake of time-boxing, I will leave this on hold for now, to focus on other features that might have higher priority than this. Will revisit soon with a fresh mind, to dig into the issue.

Base automatically changed from block-attributes to main October 24, 2022 11:09
@psrpinto psrpinto force-pushed the service-worker branch 2 times, most recently from 1f6a723 to 77b69fd Compare October 24, 2022 14:43
@adamziel
Copy link

There is currently an issue when updating the service worker: when a new version of the service worker exists, refreshing the page causes all subsequent fetch requests to fail.

I've seen this happen in https://github.com/WordPress/wordpress-wasm too! Adding this to the service worker helped:

	/**
	 * Ensure the client gets claimed by this service worker right after the registration.
	 *
	 * Only requests from the "controlled" pages are resolved via the fetch listener below.
	 * However, simply registering the worker is not enough to make it the "controller" of
	 * the current page. The user still has to reload the page. If they don't an iframe
	 * pointing to /index.php will show a 404 message instead of a homepage.
	 *
	 * This activation handles saves the user reloading the page after the initial confusion.
	 * It immediately makes this worker the controller of any client that registers it.
	 */
	self.addEventListener('activate', (event) => {
		// eslint-disable-next-line no-undef
		event.waitUntil(self.clients.claim());
	});

Also, I register the service worker like this:

const registration = await navigator.serviceWorker.register(scriptUrl);
await registration.update();

// Just in case, let's confirm the worker is already running. On Chrome
// this test sometimes fails for me, on other browsers it mostly works.

const workerRunning = await fetch('/service-worker-test');
// /service-worker-test is only served by the service worker
if(workerRunning.status !== 200) {
    window.location.reload();
}

@psrpinto
Copy link
Member Author

Thanks @adamziel!

Not too clear what this is, but seems to be required by the service worker.
@psrpinto psrpinto force-pushed the service-worker branch 3 times, most recently from 34ebe13 to 3206238 Compare November 25, 2022 17:22
@psrpinto
Copy link
Member Author

I've now managed to make the service worker updates work consistently both in Firefox and Chrome. The issue I was having seems to be related to our ServiceWorkerHandler extending hydrogen's, copy-pasting hydrogen's ServiceWorkerHandler resolves the issue somehow. I'm baffled but oh well, it works now.

However, currently the user is asked through an alert that blocks the whole page whether they want to apply the new version:

Screenshot 2022-11-25 at 17 26 13

If I remove this alert and silently update, on Chrome, sometimes the update works (the new service worker boots correctly), sometimes it doesn't. On Firefox it works consistently, so this seems to be an issue with Chrome. My guess is this is due to some timing issue, since there's asynchronous stuff going on.

In any case, I think the user should be asked if they want to update, as they might be composing a message, for example, which would be lost. However, the alert should only block the chat "window", not the whole page.

So I will proceed to implement an "alert" that only covers the chat "window".

@psrpinto
Copy link
Member Author

Thanks for the help debugging this @ashfame !

@akirk
Copy link
Member

akirk commented Nov 28, 2022

In any case, I think the user should be asked if they want to update, as they might be composing a message, for example, which would be lost. However, the alert should only block the chat "window", not the whole page.

This is a good point. Do you think we can adapt the text to explain this? Something along the lines of "If you press OK this will reload the chat, any typed messages will be gone".

So I will proceed to implement an "alert" that only covers the chat "window".

Is this necessary or could we just make it work with improved wording in the box?

@akirk
Copy link
Member

akirk commented Nov 28, 2022

I've now managed to make the service worker updates work consistently both in Firefox and Chrome. The issue I was having seems to be related to our ServiceWorkerHandler extending hydrogen's, copy-pasting hydrogen's ServiceWorkerHandler resolves the issue somehow. I'm baffled but oh well, it works now.

This is great news! Thanks for continuing the work on this!

Now that this is working, could you confirm what the service worker specifically handles?

  • Does it fix Multiple tabs open tabs?
  • You mentioned "offline support," what does it mean?

Thanks!

@akirk
Copy link
Member

akirk commented Nov 28, 2022

For the alert-replacement we can use the <dialog> element: https://caniuse.com/dialog

@psrpinto
Copy link
Member Author

Now that this is working, could you confirm what the service worker specifically handles?

I've updated the PR description with some more details on this. I've also added a screenshot of the update dialog.


Note that there's still an issue on Chrome (#140), but I'd rather merge this PR and address that issue separately.

Also note that the service worker does not actually allow having more that one instance of chatrix active (more details in PR description):

Whenever a (non-active) instance tries to read/write from/to the store, the session will be closed in the active instance. Closing the session means the user is sent to the session selection screen.

So this is unfortunate, but I think the service worker still provides value, as it prevents the user corrupting the store. Addressing this issue to make it possible to have more than one active session would need to be done upstream, in hydrogen. I'm happy to explore this if we think there is a need for it.

@psrpinto psrpinto marked this pull request as ready for review November 28, 2022 18:25
Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

Not allowing a second instance of the chat is not the best but also better than an experience where messages might be missing from the communication, making things appear broken.

Could you provide one more screenshot that shows what happens when you open an second tab? Which one is the chat that will keep working, the first one or the second one?

@ashfame
Copy link
Member

ashfame commented Nov 29, 2022

2 things come to mind:

  • What does the user see in Chatrix's first tab when a second tab is loaded?
  • Is our service worker doing more than Hydrogen's service worker or this is purely to workaround the issue we had with it not working without a manual reload?

@psrpinto
Copy link
Member Author

What does the user see in Chatrix's first tab when a second tab is loaded?

I added a screen recording to the PR description that shows the interaction. But to answer the question: the user of the first tab sees the session selection screen.

Is our service worker doing more than Hydrogen's service worker

No, our service worker has exactly the same functionality as hydrogen's, the only difference being the upgrade process, where hydrogen shows an alert() that blocks the page, and we show a <dialog> which only blocks the iframe.

@ashfame
Copy link
Member

ashfame commented Nov 29, 2022

Thanks for the screen recording, I see what happens and its good enough to avoid any confusions for now. Seems a deliberate switch out of the room in the code somewhere.

Feel free to move forward.

Re: service worker, this would mean we would have to manually catch up with Hydrogen service worker changes to be copied over to ours as they happen, right? Would be nice to not have to deal with that, eventually.

@psrpinto psrpinto merged commit 8dc9556 into main Nov 29, 2022
@psrpinto psrpinto deleted the service-worker branch November 29, 2022 13:33
@psrpinto psrpinto mentioned this pull request Dec 15, 2022
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.

Issues when app is open in multiple tabs
4 participants