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

All once handlers are being flagged as used as soon as a request is intercepted, no matter the URL #1782

Closed
4 tasks done
acidoxee opened this issue Oct 22, 2023 · 4 comments · Fixed by #1822
Closed
4 tasks done
Labels
bug Something isn't working needs:triage Issues that have not been investigated yet. scope:node Related to MSW running in Node
Milestone

Comments

@acidoxee
Copy link

acidoxee commented Oct 22, 2023

Prerequisites

Environment check

  • I'm using the latest msw version
  • I'm using Node.js version 14 or higher

Node.js version

v18.18.0

Reproduction repository

https://stackblitz.com/edit/vitest-dev-vitest-xa3evs?file=test%2Fbasic.test.ts

Reproduction steps

Just let the tests run (which should happen as soon as you land on the page), the green test highlights that it shouldn't behave that way.

Current behavior

I've got a detailed answer here already, but for good measure:

  • register a once handler, for instance for https://test.local/foo/trpc/getById
  • perform a request for another URL, for instance for https://test.local/bar/trpc/getByName
  • perform a request for the supposedly mocked URL (https://test.local/foo/trpc/getById), see that it's not properly handled and that the once handler had already been flagged as used

I've seen this behavior in 0.0.0-fetch.rc-23 and 0.0.0-fetch.rc-24.

Expected behavior

The once handler registered for https://test.local/foo/trpc/getById should not have been flagged as used after the first request, because it didn't match. In the reproduction repo, there should even still be one once unused handler in the pool, since only one request to https://test.local/foo/trpc/getById has been triggered, whereas there were two once handlers registered for that URL.

@acidoxee acidoxee added bug Something isn't working needs:triage Issues that have not been investigated yet. scope:node Related to MSW running in Node labels Oct 22, 2023
@kettanaito
Copy link
Member

Thanks for reporting this, @acidoxee! I'm trying to reproduce this behavior in #1781, so far without luck. The tests are passing, and non-matching handlers are not marked as used.

@kettanaito kettanaito added this to the Post-2.0 milestone Oct 22, 2023
@acidoxee
Copy link
Author

acidoxee commented Oct 23, 2023

It seems you're using lower-level primitives to test the behavior, whereas in my reproduction I'm using plain fetch requests. Maybe there's something wrong in-between? Sorry if that doesn't make sense, I'm not familiar with MSW's internals at all 🙈

Or maybe you're using a different Node version? I'm having the problem on 18.17.1, 18.18.0 and 18.18.2, I haven't tried with other versions.

I'm also using Vitest, not Jest, both in my own repo and in the reproduction one. I don't have any idea whether that could influence the results.

@joshkel
Copy link
Contributor

joshkel commented Oct 30, 2023

I ran into this today.

It seems that the problem is here:

// Immediately mark the handler as used.
// Can't await the resolver to be resolved because it's potentially
// asynchronous, and there may be multiple requests hitting this handler.
this.isUsed = true
const parsedResult = await this.parse({
request: args.request,
resolutionContext: args.resolutionContext,
})
const shouldInterceptRequest = this.predicate({
request: args.request,
parsedResult,
resolutionContext: args.resolutionContext,
})
if (!shouldInterceptRequest) {
return null
}

It immediately marks the handler as used, to prevent two back-to-back requests from both going through. However, parsing the request, to determine if the handler should be invoked, is asynchronous (as needed by the GraphQL handler). There appears to be no way to have both "immediately enforce 'once'" and "asynchronously determine whether it's used", unless you do something such as changing isUsed to a promise or serializing access to it via a mutex?

Here's a test in handleRequest.test.ts that should suffice to reproduce the issue.

(collapsed for a shorter issue description)
it('does not mark non-matching one-time handlers as used', async () => {
  const { emitter } = setup()

  const oneTimeHandler = http.get(
    '/resource',
    () => {
      return HttpResponse.text('One-time')
    },
    { once: true },
  )
  const anotherHandler = http.get(
    '/another',
    () => {
      return HttpResponse.text('Another')
    },
    { once: true },
  )
  const handlers: Array<RequestHandler> = [oneTimeHandler, anotherHandler]

  const requestId = uuidv4()
  const firstResult = await handleRequest(
    new Request('http://localhost/another'),
    requestId,
    handlers,
    options,
    emitter,
    callbacks,
  )

  expect(await firstResult?.text()).toBe('Another')
  expect(oneTimeHandler.isUsed).toBe(false)
  expect(anotherHandler.isUsed).toBe(true)

  const secondResult = await handleRequest(
    new Request('http://localhost/resource'),
    requestId,
    handlers,
    options,
    emitter,
    callbacks,
  )

  expect(await secondResult?.text()).toBe('One-time')
  expect(anotherHandler.isUsed).toBe(true)
  expect(oneTimeHandler.isUsed).toBe(true)
})

@kettanaito
Copy link
Member

Released: v2.0.2 🎉

This has been released in v2.0.2!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

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:triage Issues that have not been investigated yet. scope:node Related to MSW running in Node
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants