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

Unregister self when not activated? #96

Closed
kentcdodds opened this issue Apr 9, 2020 · 9 comments · Fixed by #99
Closed

Unregister self when not activated? #96

kentcdodds opened this issue Apr 9, 2020 · 9 comments · Fixed by #99
Labels
bug Something isn't working needs:discussion

Comments

@kentcdodds
Copy link
Contributor

I have MSW working for a project on localhost:3000, then I close that project and open a new one which does not have MSW and that starts up on localhost:3000 (the default for create-react-app based apps) and MSW hangs around, logs in the console, and generally confuses people.

Can we have MSW's service worker unregister itself when it's not activated when the app starts up? I'd be fine if this is opt-in.

@kettanaito
Copy link
Member

kettanaito commented Apr 9, 2020

This is a good point. I've been stumbling upon this myself, with the service worker at place where it shouldn't have been. At the moment it does deactivate the mocking upon window unload, but I see there is no logic to unregister the worker:

https://github.com/open-draft/msw/blob/75d2562065d0315911cbca718477d7ef4df684e8/src/composeMocks.ts#L29-L34

I find it a good idea to unregister the worker, for it's bound to a host (I believe it's not even bound to the port), yet I have one concerns:

  • When should the service worker be unregistered?

I'll try to perform that on the same beforeunload window event, for that is a reliable way of knowing that window is being closed. My only worry is that there is no guarantee that the service worker unregister promise will resolve prior to the window actually unloading. That may lead into some unexpected scenarios, I suppose. I'd like to test how it behaves.

Need to keep in mind that there may be multiple clients listening to the same mocking Service Worker. The latter must not unregister when one window closes, but, ideally, when all its clients are closed.

@kentcdodds
Copy link
Contributor Author

I think that's a good approach.

Another thing that might work is when the service worker starts up, it starts a timer and if it's not activated after a certain amount of time, then it unregisters itself? I can see why this may need to be opt-in though.

@kettanaito
Copy link
Member

I gave it a quick manual test and unregistering the service worker on beforeunload seems to wield an expected result.

Timer sounds good, yet I'm afraid it creates a side-effect that is even harder to reason about than sw.unregister(). I can imagine it being subjected to a race condition (what finishes first: page unload or timer's callback?), and can't say from the top of my head if you reload the page with an action in a timer, would that action still execute after the page loads?

@kentcdodds
Copy link
Contributor Author

Maybe on start up it can post a message to the client and if it doesn't get a response then it can unregister itself?

@kettanaito
Copy link
Member

That's a possibility. Still I'd like to delegate registration responsibility to the client-side library, and not the service worker. This way there is a single point of control, treating the service worker as a ready-only artifact, if that makes sense.

@kettanaito kettanaito added bug Something isn't working needs:discussion labels Apr 15, 2020
@kettanaito
Copy link
Member

kettanaito commented Apr 18, 2020

Released in 0.13.0.
This release comes with a change to mockServiceWorker.js, so please make sure to:

$ npm install msw@latest
$ npx msw init <PUBLIC_DIR>

Service Worker instance should now unregister itself when the last controlled client is closed. That is achieved by keeping a map of controlled clients internally in the worker's scope, and removing a client by ID once it fires a beforeunload window event.

Eventually, I added a self-terminating logic to the worker, despite my previous comment. Service Worker is a reliable context to unregister itself, as it's persistent between clients' sessions, and keeps running even if you close the client. It's enough for the client to signal it's being closed, and there is no race condition for the worker to unregister itself, as the handling of unregistration is not bound to the client.

@kentcdodds
Copy link
Contributor Author

Tested this out and it works perfectly. Thanks!

@adp-source
Copy link

This isn't working for me. I'm still seeing the mockserviceworker in Dev Tools, even after restarting my computer.

"msw": "^0.21.2"
Chrome Version 88.0.4324.96 (Official Build) (x86_64)

Service worker is running on two clients locally, one in HTML file and one in React app

@kettanaito
Copy link
Member

kettanaito commented Jan 29, 2021

@adp-source, could you please elaborate more on how you see the worker? What page do you check the worker on? What state the worker is in? A screenshot of it in the "Application" tab could be useful.

Before you do, please update to the latest version of msw (0.26.0 at the moment) and re-instantiate its worker script (follow the instructions in the browser's console). You are a couple of versions behind and a lot of things have changed since then.

If the worker is active, that means there is a client that's still controlled by it. When the last controlled client is closed, the worker unregisters itself. We have this behavior tested, but I never deny a chance for error.

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

Successfully merging a pull request may close this issue.

3 participants