feature: remove listeners on stop #244
Conversation
There was a problem hiding this comment.
Thank you for these changes, @marcosvega91! I've given them a look, have a few suggestions. Let me know what you think about those.
src/setupWorker/start/createStart.ts
Outdated
| if (worker.state !== 'redundant') { | ||
| // Notify the Service Worker that this client has closed. | ||
| // Internally, it's similar to disabling the mocking, only | ||
| // client close event has a handler that self-terminates | ||
| // the Service Worker when there are no open clients. | ||
| worker.postMessage('CLIENT_CLOSED') | ||
| } | ||
| } | ||
| window.addEventListener('beforeunload', beforeUnload) |
There was a problem hiding this comment.
If we adopt the context.addListener() pattern, this can be simplified, as we won't have to repeat the beforeUnload twice.
There was a problem hiding this comment.
I have only this doubt, why you said that we do this twice?
There was a problem hiding this comment.
I'm looking to integrate msw with BigTest and running into this issue. Our use-case is that we have two frames that share an origin: a control frame where test instructions are run, and an app frame that actually contains the application under test.
This is causing instances to pile up in our control frame in between test cases. Is there anything I can do to push this forward?
@kettanaito I've added some thoughts to the PR, and would be happy to push this across the finish line since it would be very beneficial for us to land this.
| @@ -8,6 +8,14 @@ export interface ComposeMocksInternalContext { | |||
| worker: ServiceWorker | null | |||
| registration: ServiceWorkerRegistration | null | |||
| requestHandlers: RequestHandler<any, any>[] | |||
| events: { | |||
There was a problem hiding this comment.
why stuff this into an events namespace?
context.addListener()
context.removeAllListers()seems a lot more simple and in keeping with most event listener apis out there. Putting it under events seems like overkill especially since the events object isn't peeled off anywhere and passed around of its own accord.
There was a problem hiding this comment.
it makes sense but we did in this way to separate events from other context parameters
There was a problem hiding this comment.
I don't see how grouping the events-related properties/methods in the events key is an overkill. It's perfectly fine, I'd leave it like that. It makes events references from the context much more apparent, and gives us a single point of control if we decide to refactor this functionality in the future.
src/setupWorker/start/createStart.ts
Outdated
| if (worker.state !== 'redundant') { | ||
| // Notify the Service Worker that this client has closed. | ||
| // Internally, it's similar to disabling the mocking, only | ||
| // client close event has a handler that self-terminates | ||
| // the Service Worker when there are no open clients. | ||
| worker.postMessage('CLIENT_CLOSED') | ||
| } | ||
| }) | ||
| } | ||
| context.events.add(window, 'beforeunload', beforeUnload) |
There was a problem hiding this comment.
Instead of using a separate callback, why not just call stop() at this point. This guarantees that there is a single codepath for starting and stopping, and that stopping happens automatically on a window beforeunload.
| let listeners: MSWEventListener[] | ||
| const removeLiteners = () => { | ||
| listeners.forEach((listener) => | ||
| listener.handler.removeEventListener(listener.type, listener.listener), | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Rather than re-invent the wheel here, wouldn't it make sense to just pass in the context to activateMocking? That way, we can re-use the automated teardown logic from there.
In fact, since in this PR, both are actually adding glue to remove listeners added to window by createBroadCastChannel at the time the context is stopped, and these are the only cases of using createBroadCastChannel, wouldn't it make sense to remove createBroadCastChannel entirely and just use this mechanism?
There was a problem hiding this comment.
I have done in this way because activateMocking listeners are removed asap, when the promise is resolved. So I thought to not add to context listeners
There was a problem hiding this comment.
Even in that case, I think it's best to still put that onto the context in so that it can re-use the global teardown in the event that the window is torn down, or stop is called before the immediate message is received.
For example, I believe there is a race condition that will happen if:
addMessagelistener()is called- user calls
stop() - message is received.
In that case, the listener will still be dispatched when it shouldn't be.
I think we can have the best of both worlds by implementing a once() method on the context, similar to EventEmitter#once which lets us have the best of both worlds. It removes itself immediately, but it also uses the context-bound event registration mechanism that won't overstay its welcome.
Here is an example implementation https://github.com/cowboyd/msw/blob/82fa2ad486f0030e6e4fb9f352f295cedb45de92/src/setupWorker/setupWorker.ts#L62
As a bonus, I made it return a promise so that you can now use simple async functions to work with message dispatch and response https://github.com/cowboyd/msw/blob/82fa2ad486f0030e6e4fb9f352f295cedb45de92/src/setupWorker/setupWorker.ts#L62-L100
Where this approach really shines is in things like the integrity check which can now be written as a simple async function. Thus this:
export function requestIntegrityCheck(
serviceWorker: ServiceWorker,
): Promise<ServiceWorker> {
let listeners: MSWEventListener[]
// Signal Service Worker to report back its integrity
serviceWorker.postMessage('INTEGRITY_CHECK_REQUEST')
const removeLiteners = () => {
listeners.forEach((listener) =>
listener.handler.removeEventListener(listener.type, listener.listener),
)
}
return new Promise((resolve, reject) => {
listeners = addMessageListener(
'INTEGRITY_CHECK_RESPONSE',
(message) => {
const { payload: actualChecksum } = message
// Compare the response from the Service Worker and the
// global variable set by webpack upon build.
if (actualChecksum !== SERVICE_WORKER_CHECKSUM) {
return reject(
new Error(
`Currently active Service Worker (${actualChecksum}) is behind the latest published one (${SERVICE_WORKER_CHECKSUM}).`,
),
)
}
removeLiteners()
resolve(serviceWorker)
},
() => {
removeLiteners()
reject()
},
)
})
}becomes this:
export async function requestIntegrityCheck(
context: SetupWorkerInternalContext,
serviceWorker: ServiceWorker,
): Promise<ServiceWorker> {
// Signal Service Worker to report back its integrity
serviceWorker.postMessage('INTEGRITY_CHECK_REQUEST')
const { payload: actualChecksum } = await context.once('INTEGRITY_CHECK_RESPONSE')
// Compare the response from the Service Worker and the
// global variable set by webpack upon build.
if (actualChecksum !== SERVICE_WORKER_CHECKSUM) {
throw new Error(
`Currently active Service Worker (${actualChecksum}) is behind the latest published one (${SERVICE_WORKER_CHECKSUM}).`,
)
}
return serviceWorker
}Which seems like a pretty nice win.
|
@marcosvega91 @kettanaito fwiw, I've implemented these suggestions here master...cowboyd:pr/remove_listeners_on_stop |
|
for me suggestions are valid 👍 . Thanks for your time on this 😄 |
|
I'm stunned by the quality of work you've done here, folks. I'm cherry-picking the commits from @cowboyd into your pull request, @marcosvega91, and will provide some polishing on top. |
|
I've added the ability for Trying to get the This most likely means that the
|
|
Thanks @kettanaito for the help. I'll look into it tomorrow because today i'm not at home unfortunately |
|
@kettanaito I'm also trying to have a look, but don't seem able to run the integration tests. I'm consistently getting this error (on both node 12, and 14): |
Oops, didn't see this #188 |
|
So strange: This test passes for me locally. |
|
I'm a bit confused, I can't reproduce the test failure locally, and it looks like what's failing in CI is actually @kettanaito do you think these are related? |
|
@kettanaito Unless I'm mistaken, it looks like the I've tested on node@12 and node@14 with the result that all integration tests pass. 🤷 |
|
I have tried on my local env and all tests are passing using node@12 |
|
The issue on CI I think is regarding to this #276 |
|
I have simply run again the pipeline and not it's working. We have to move forward on the other issue to solve definitively |
|
Thank you for running that test locally, fellows. I will try to run it once more. Wonder what fails it for me. |
|
I'm not sure but I think that the issue that you had could be regarding to #188. I'm not sure but it could make sense |
|
Rebased upon the latest If this passes on the CI, I'll dive into the difference between CI and my local machine. So far the versions of node/etc. should be the same. What is apparent is that the I wonder if this can be causing any trouble. However, it works for you, folks, and I presume you are running the same old version of Puppeteer. |
|
I've tried to make There is definitely an evaluation issue here, as it seems whenever I try to call Although the test passes on CI, I cannot treat it as stable until I reproduce it locally. I'm leaning towards having this test suite skipped. @marcosvega91, do you have any suggestions in my case, please? |
When the user run woker.stop() all listners attached will be removed. I noticied that `activeMocking` and `requestIntegrityCheck` were useful only at the beginning, so after resolve/reject I have removed both listeners
|
Ok now I have the issue like you. As you can see also the CI is failing |
|
I'm marking this test as skipped and referencing this PR. |
|
I have fixed the test, when you merge from master you changed |
|
@marcosvega91, I'm terribly sorry, just forced pushed. Could you please push that fix commit once more? 🙏 |
|
It is not a problem 😃 . I have pushed the fix 🥳 , let me know |
|
Thank you! I'm preserving that test, but marking it as skipped. It's evident enough to come back to it later and try to resolve it. I'd start from bumping dependencies versions before the next release, perhaps there's some behavior change in Puppeteer. Thank you for the awesome work! |
|
There's one issue I'm experiencing that's not necessarily related to these changes, but I'd like to investigate it before merging this to be safe. Issue reproduction
Issue descriptionIt appears that although we are clearing the listeners after unload/ If you observe the browser logs, you can see that the first response contains the old data (before refresh), while the second response contains the updated data. However, since the first response is delivered first, it's being used in the app. Do you have some thoughts on how to tackle this? I have a suspicion that if we remove the listener of this event on Fast reload it would work: const remove = context.events.addListener(
navigator.serviceWorker,
'message',
handleRequestWith(context, resolvedOptions),
)
if (FAST_REFRESH) {
remove()
}The question is how to know that Fast refresh is at place? |
|
How I'm able to solve that is by keeping a global reference to the let remove: any
export const createStart = (context: SetupWorkerInternalContext) => {
/**
* Registers and activates the mock Service Worker.
*/
return function start(options?: StartOptions) {
// ...
if (remove) {
console.warn('Code evaluated again, but the listener is present. Remove.')
remove()
}
// Store the global reference.
remove = context.events.addListener(
navigator.serviceWorker,
'message',
handleRequestWith(context, resolvedOptions),
)Since Fast refresh re-evaluates function's scope, if you declare However, I dislike this approach because an obsolete event handler issue on Fast refresh is affecting all the handlers, not just this one. It seems this should be solved on the |
Fast Refresh solutionWhat I found to be consistently solving the Fast refresh issue:
|
|
I'm also experiencing a stale data issue on the server-side of Next JS (with |
There was a problem hiding this comment.
Thank you for the superb changes, @marcosvega91, @cowboyd! Approved.
Hey I have partially closed #217.
When the user run
woker.stop()all listeners attached will be removed.I noticed that
activeMockingandrequestIntegrityCheckwere useful onlyat the beginning, so after resolve/reject I have removed both of them.
I have added a new test to be sure that everything work as I'm expected