-
Notifications
You must be signed in to change notification settings - Fork 723
Update MSW usage in "example-intro" #1327
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
Conversation
✅ Deploy Preview for testing-library ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kettanaito! It's nice to see that this is happening :)
At the moment, RTL supports Node 14>, maybe instead of changing the current example we can add new ones for the new version of msw
?
@MatanBobi, would it make sense to upgrade RTL itself? Node.js v14 and v16 are EOL. I think it's best RTL migrates to the LTS, which is v18, which is also compatible with MSW 2.0. |
We have that planned as part of DTL major upgrade that's currently in alpha, once we'll merge it, we'll also bump this one :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, and congrats with the release @kettanaito !
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
@kettanaito do you want me to put the old examples too? Since we're not yet enforcing Node 18, we should probably keep both examples there. |
@MatanBobi, I'm concerned that will be confusing for the reader. If RTL doesn't support Node.js 18 yet, leave the old examples but add a note that recommends using MSW 1.x because that's the last version compatible with Node.js 14-16. I wouldn't feature new examples because people can't use them anyway (RTL locks them to old versions of Node.js incompatible with MSW). |
It's not that RTL users can't use Node 18+, it's that they can use Node 14-16. We'll be changing that soon. |
Because there are already some requests to update this I think we should merge this. |
Sounds like a good idea to me :) |
Great news! I've heard from people they were confused about the example featuring MSW 1. I'm so excited to see this updated! 🚀 Also, thanks for the pointers on Node 18 support. I will keep that in mind if I get any questions about RTL + Node 18. |
I'm not sure if there are plans to add this, but setup for this example with jest also requires that the tests are run in the jsdom environment, which causes msw to require the following polyfill: https://mswjs.io/docs/migrations/1.x-to-2.x#requestresponsetextencoder-is-not-defined-jest as well as the PS The polyfill doesn't even work, so I feel like msw is prohibitively complicated to use even if all of this was detailed in the docs. I feel like it shouldn't be recommended until this issue is resolved, probably over at jsdom/jsdom#2524 |
@@ -72,15 +72,15 @@ See the following sections for a detailed breakdown of the test | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I wanted to create a warning, but I noticed that we don't explicitly mention that the example uses MSW so I used a note.
:::note | |
The example uses [MSW (Mock Service Worker)](https://mswjs.io/) to mock HTTP requests. | |
MSW requires Node.js 18 or later. | |
::: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, it does mention it below the example.
Because I can't add a comment there, the lines at https://github.dev/kettanaito/testing-library-docs/blob/dbd489c8fa229dd43473daf7fd2d205c66fc3288/docs/react-testing-library/example-intro.mdx#L120-L122 should be removed.
:::note | |
We recommend using the [Mock Service Worker (MSW)](https://github.com/mswjs/msw) library | |
to declaratively mock API communication in your tests instead of stubbing | |
`window.fetch`, or relying on third-party adapters. | |
MSW requires Node.js 18 or later. | |
::: |
Closing in favor of #1378. Thanks @kettanaito for working on this :) |
@skondrashov, we are trying to make the experience with Jest better through https://github.com/mswjs/jest-fixed-jsdom. Feel free to try it and let me know if it fixes the problems. This environment is meant to replace all the suggestions in the current docs, afaik.
The confusion is understandable. I don't recommend Jest because it's an old tool that fails to adapt to the pace with which JavaScript evolves. That doesn't necessarily means MSW doesn't work in Jest. In fact, you have examples with plain Jest as well as Jest+JSDOM working. Take a look at those and see what you have to adjust in your Jest config.
Polyfill certainly works, and the I'm saying this to stress that neither of those tools considers this a bug so don't count on it ever getting resolved. This is why I recommend tools like Vitest and HappyDOM that manage to respect your globals better. They aren't perfect but they are years ahead in the testing framework field than alternatives. |
This also brings forth a far larger discussion of browser-like environments and how, imo, they have no place in 2024. You should be testing browser code in the browser. Browsers are far nicer to automate and cheaper to spawn than ever before. JSDOM was created 14 years ago to tackle terrible browser automation experience. It's not terrible anymore. In fact, it's quite fantastic. With both Playwright and Cypress investing in component-level testing, I hope to see the day when everybody will move away from hacks to actual in-browser testing. |
@kettanaito Thank you for the work you've been doing, and sorry for my late reply. I found the time to give RTL with MSW another shot using the new test environment, and I'm still unable to get the basic example to work. Both I agree largely with your points on using browsers rather than fake environments. I disagree with the way you see the projects involved in this problem, because jsdom do in fact view not having every polyfill as a bug - their whole thing is trying to recreate a browser environment. Jest gives you a base javascript environment, and then the responsibility of jsdom is to make it browser-like. This makes perfect sense to me, and I feel like I'm aligned with the way the Jest developers see it and the way the jsdom developers see it. Yes, jsdom should provide definitions for browser features that msw relies on, but the jsdom developers agree with that: jsdom/jsdom#2524 (comment) The best thing I think I can offer you is my perspective as a mostly lazy person who only codes for work at my job. I think the overwhelming majority of people who are going to be trying to set this up are lazy people who only code for work at their job. We would greatly benefit from having a document that exhaustively explains the steps needed to get msw working on that page. |
@skondrashov, please use https://github.com/mswjs/jest-fixed-jsdom if you want to use MSW in Jest+JSDOM combo. MSW works fine in raw Jest. Here are a few reference examples:
I don't believe this code snippet is ever mentioned anywhere. It's
I have hard feelings about this. If you get
That's what we've published |
@kettanaito I did in fact use |
Something is seriously mixed here. You should never use If you found that some information in MSW led you to believe you can use Edit: I'm really curious how come you are not getting runtime exceptions when using |
You're right, I didn't notice that the examples use two different names. I went to the msw readme after being unable to get the RTL docs to work, and thought maybe that usage example would work. I certainly didn't think about whether setupWorker would work in Node.js. I believe I was getting the same " |
The only thing to watch out is that MSW requires Node.js 18 or later. If RTL requirements contradict this, we shouldn't update the examples.