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

feat!: adopt the global Fetch API #1436

Merged
merged 211 commits into from Oct 23, 2023
Merged

feat!: adopt the global Fetch API #1436

merged 211 commits into from Oct 23, 2023

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented Oct 17, 2022

This is a breaking change.

Migration guide

Roadmap

pnpm test:node rest-api/request/body/body-form-data.node.test.ts --detectOpenHandles

Jest has detected the following 1 open handle potentially keeping Jest from exiting:

  ●  MESSAGEPORT

      154 |     return this.predicate(
      155 |       request,
    > 156 |       await this.parse(request.clone(), resolutionContext),
          |                                ^
      157 |       resolutionContext,
      158 |     )
      159 |   }

      at RestHandler.test (../../src/handlers/RequestHandler.ts:156:32)
      at ../../src/utils/getResponse.ts:34:20
          at Array.map (<anonymous>)
      at filterAsync (../../src/utils/getResponse.ts:21:44)
      at getResponse (../../src/utils/getResponse.ts:33:34)
  • Resolve the wrong response clone in serializeResponse:
  ✓  60 [chromium] › msw-api/setup-worker/printHandlers.test.ts:14:5 › lists rest request handlers (1.7s)
Error [TypeError]: Failed to execute 'clone' on 'Response': Response body is already used
    at serializeResponse (webpack://msw/./lib/index.js?:548:34)
    at RestHandler.log (webpack://msw/./lib/index.js?:864:34)
  • Resolve xhr.onload being called twice (XMLHttpRequest: "onload" is called twice interceptors#359)
  • Publish MSW as CJS/ESM for both browser and Node.js.
  • Decide how to distribute type definitions because you may want to access SetupWorkerApi in Node.js module to extend window when working with Playwright.
  • Memory leak in node tests:
(node:3158) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit
    at EventTarget.[kNewListener] (node:internal/event_target:514:17)
    at EventTarget.[kNewListener] (node:internal/abort_controller:183:24)
    at EventTarget.addEventListener (node:internal/event_target:623:23)
    at Request.clone (node:internal/deps/undici/undici:7303:23)
    at RestHandler.<anonymous> (msw/lib/core/handlers/RequestHandler.js:109:53)
    at Generator.next (<anonymous>)
    at msw/lib/core/handlers/RequestHandler.js:53:61
    at new Promise (<anonymous>)
    at __async (msw/lib/core/handlers/RequestHandler.js:37:10)
    at RestHandler.run (msw/lib/core/handlers/RequestHandler.js:103:12)
    at msw/lib/core/utils/getResponse.js:66:35
    at Generator.next (<anonymous>)
    at fulfilled (msw/lib/core/utils/getResponse.js:23:24)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 17, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 90b854a:

Sandbox Source
MSW React Configuration
Demo of bug Issue #1427

@kettanaito
Copy link
Member Author

@thepassle, yes. I forgot for a moment how awesome Playwright is. Would you like to open a pull request with those?

@weyert
Copy link

weyert commented Oct 18, 2023

@luchsamapparat btw, that's wrong you shouldn't use beforeAll and expect on setup files and if you are environment is jsdom in expected that the AbortController was patch by jsdom if you don't want that then you need to use enviroment: node

here you go -> https://codesandbox.io/p/sandbox/funny-flower-lfqywh?file=%2Fsrc%2Fabort-controller.test.ts%3A9%2C42

But wouldn't it still be a problem for when you use jsdom to test React/web code and use mew to mock some external web servers? You would still have the issue of AbortSignal being using msws instead of Node's EventTarget that Node is expecting?

@luchsamapparat
Copy link

@weyert exactly.

@dbritto-dev
Copy link

@weyert shouldn't be a problem since you can override the AbortController from jsdom instance with the one from node but if you use environment: jsdom all the globals are patched even before setup files. BTW, you can run some tests with environment: jsdom and other ones with environment: node

@christoph-fricke
Copy link
Contributor

@weyert shouldn't be a problem since you can override the AbortController from jsdom instance with the one from node but if you use environment: jsdom all the globals are patched even before setup files. BTW, you can run some tests with environment: jsdom and other ones with environment: node

@kettanaito @luchsamapparat @dbritto-dev I went into the inner workings of Node.js to figure out why this error occurs in JSDOM environments and how we can prevent it to solve it for real, not just the symptoms. I found a rather simple solution that I have implemented in in PR #1779.

christoph-fricke and others added 2 commits October 20, 2023 11:48
…s from "setMaxListeners" in jsdom (#1779)

Co-authored-by: Artem Zakharchenko <kettanaito@gmail.com>
@kettanaito
Copy link
Member Author

📦 v0.0.0-fetch.rc-24

@thepassle
Copy link
Contributor

thepassle commented Oct 20, 2023

Is there any particular reason why the tsconfig target is set to es6 and not something slightly more modern? The es6 target turns async/await into generator functions wrapped in __async, and a bunch of other seemingly unnecessary stuff:
image

Im asking, because I've had to debug something in MSW several times now, and its extremely painful to debug code like this. Perhaps we could consider raising the target to something a bit more contemporary?

edit: actually it may be esbuild doing this? TS's output names it __awaiter instead. In any case, can we just ship more modern code? async/await and spread has good node/browser support for a long time now

@kettanaito
Copy link
Member Author

@thepassle, absolutely agree and absolutely welcome a pull request! 🙏

Co-authored-by: Artem Zakharchenko <kettanaito@gmail.com>
@thepassle
Copy link
Contributor

@thepassle, absolutely agree and absolutely welcome a pull request! 🙏

#1780

@acidoxee
Copy link

acidoxee commented Oct 20, 2023

I've spent some time yesterday migrating away about a thousand handlers to the new fetch API (currently on the fresh 0.0.0-fetch.rc-24), and I must say I'm liking this new API a lot.

BUT, I've been stuck all day on some failing tests using the new API. Not a lot though, which was surprising, and it turns out tests that fail are the ones that used res.once() (now migrated to the { once: true } option). While requests were properly intercepted and handled in v1, with the v2 API it's not the case anymore.

I'm NOT saying all once handlers don't ever match no matter the conditions. But I've narrowed it down to this: if I set up all of my handlers at the beginning of my test (which is often the case, because I'm not directly firing requests myself, it's lower-level code that's not directly accessible in my test, so I have no way to interlace MSW once handlers and fetch-related code one by one), and if there is at least one request that's not related to a once handler that fires away, all of my once handlers are immediately switched to { isUsed: true } and don't ever match afterwards, even if they did in v1.

The current v1 docs state the following about one-time overrides:

Request handler override can be used to handle only the next matching request. After a one-time request handler has been used, it never affects the network again.

There's also this code block, which states that the first matching request will receive the mocked response:

rest.get('/book/:bookId', (req, res, ctx) => {
  // The first matching "GET /book/:bookId" request
  // will receive this mocked response.
  return res.once(
    ctx.status(500),
    ctx.json({ message: 'Internal server error' }),
  )
})

I've created a quick reproduction here: https://stackblitz.com/edit/vitest-dev-vitest-xa3evs?file=test%2Fbasic.test.ts&view=editor. The test is green since I wanted to validate what I'm saying. Since the configured once handlers don't match the first request, they're not supposed to be flagged as used, and they should be kept "alive" for future potential matches, until one finally does. In my reproduction, there should even still be one once handler in the pool that should be unused, since only the first one should have been consumed.

Can you confirm this is a bug, and not a change in behavior expected in v2? Since I couldn't find any information about such a change, and since it would be extremely breaking for people who can't intertwine individual requests and once handlers' setup, I'm definitely leaning on the bug side 😉

@kettanaito
Copy link
Member Author

Thanks for reporting this, @acidoxee.

Can you confirm this is a bug, and not a change in behavior expected in v2?

I confirm this is a bug. I will do my best to address it but it may be after the 2.0 release. Please, could you open a new issue about it? Thanks.

@acidoxee
Copy link

Here you go @kettanaito: #1782. Thanks a lot for all your hard work! 🙏

@weyert
Copy link

weyert commented Oct 22, 2023

@weyert shouldn't be a problem since you can override the AbortController from jsdom instance with the one from node but if you use environment: jsdom all the globals are patched even before setup files. BTW, you can run some tests with environment: jsdom and other ones with environment: node

@kettanaito @luchsamapparat @dbritto-dev I went into the inner workings of Node.js to figure out why this error occurs in JSDOM environments and how we can prevent it to solve it for real, not just the symptoms. I found a rather simple solution that I have implemented in in PR #1779.

Yes, I totally agree I could use different environment depending on the need. Only I never have been able to get the /* @jest-environment jsdom */ docblock comment to work.

I haven't gotten it working yet in my unit tests in Jest for Next.js against newer than r20 versions. Wondering if I need to rewrite my tests or something 🤔 Everything is timing out in r24. I will play around tomorrow. I was thinking of switching to Vitest but not sure if that's a good idea compared to next/js+jest.

@dbritto-dev
Copy link

@weyert are you using jest or vitest?

@kettanaito kettanaito merged commit 1033f65 into main Oct 23, 2023
9 checks passed
@kettanaito kettanaito deleted the feat/standard-api branch October 23, 2023 07:45
@thepassle
Copy link
Contributor

🎉

@kettanaito
Copy link
Member Author

Released: v2.0.0 🎉

This has been released in v2.0.0!

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