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

Update MSW usage in "example-intro" #1327

Closed
wants to merge 2 commits into from

Conversation

kettanaito
Copy link
Contributor

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.

@netlify
Copy link

netlify bot commented Oct 26, 2023

Deploy Preview for testing-library ready!

Name Link
🔨 Latest commit dbd489c
🔍 Latest deploy log https://app.netlify.com/sites/testing-library/deploys/655659669172b60008d4d8ab
😎 Deploy Preview https://deploy-preview-1327--testing-library.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@MatanBobi MatanBobi left a 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?

@kettanaito
Copy link
Contributor Author

@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.

@MatanBobi
Copy link
Member

@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 :)

Copy link
Member

@timdeschryver timdeschryver left a 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 !

docs/react-testing-library/example-intro.mdx Outdated Show resolved Hide resolved
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
@MatanBobi
Copy link
Member

@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.

@kettanaito
Copy link
Contributor Author

@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).

@MatanBobi
Copy link
Member

@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.

@timdeschryver
Copy link
Member

Because there are already some requests to update this I think we should merge this.
We can add a warning on this page that MSW requires Node.js v18+.
What do you think?

@MatanBobi
Copy link
Member

Because there are already some requests to update this I think we should merge this. We can add a warning on this page that MSW requires Node.js v18+. What do you think?

Sounds like a good idea to me :)

@kettanaito
Copy link
Contributor Author

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.

@skondrashov
Copy link

skondrashov commented Feb 8, 2024

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 undici package mentioned at that page. The example page doesn't currently mention any of this and it's led to a very complicated journey to try to add tests to my project. My personal experience has been that even though msw seems like a very cool project, they more or less state that you shouldn't use it with jest for this reason, while RTL recommends jest, which leads to a pretty confusing experience when msw is recommended in the docs here, and I wish that the example had used something I could get up and running in a shorter amount of time, even if the old way of doing mocks isn't as good.

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

Copy link
Member

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.

Suggested change
:::note
The example uses [MSW (Mock Service Worker)](https://mswjs.io/) to mock HTTP requests.
MSW requires Node.js 18 or later.
:::

Copy link
Member

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.

Suggested change
:::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.
:::

@MatanBobi
Copy link
Member

Closing in favor of #1378. Thanks @kettanaito for working on this :)

@MatanBobi MatanBobi closed this Apr 4, 2024
@kettanaito
Copy link
Contributor Author

@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.

My personal experience has been that even though msw seems like a very cool project, they more or less state that you shouldn't use it with jest for this reason, while RTL recommends jest, which leads to a pretty confusing experience when msw is recommended in the docs here, and I wish that the example had used something I could get up and running in a shorter amount of time, even if the old way of doing mocks isn't as good.

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.

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.

Polyfill certainly works, and the jest-fixed-jsdom is largely based on it. The Jest issue you've linked is unlikely to be resolved. Jest and JSDOM are broken by design, as they will never rely on Node.js global APIs even if those are present in both the browser and Node.js. A valid JavaScript code fails in JSDOM. That looks like a broken library to me but when I reported it to the maintainers I was kindly told that "I don't understand the goal behind the project."

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.

@kettanaito
Copy link
Contributor Author

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 kettanaito deleted the patch-1 branch April 4, 2024 15:10
@skondrashov
Copy link

skondrashov commented Jun 2, 2024

@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 import { setupServer } from 'msw/node' as suggested by the RTL docs and import { setupServer } from 'msw/browser' as suggested by the msw github readme are invalid imports after npm i msw (which installs msw@2.3.1 as of this writing).

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.

@kettanaito
Copy link
Contributor Author

@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:

  • MSW + Jest
  • MSW + Jest/JSDOM (I believe this hasn't been updated to jest-fixed-jsom yet so you can drop jest.polyfills.js from this).

import { setupServer } from 'msw/browser' as suggested by the msw github readme

I don't believe this code snippet is ever mentioned anywhere. It's from 'msw/node', that's the correct import.

Yes, jsdom should provide definitions for browser features that msw relies on, but the jsdom developers agree with that:

I have hard feelings about this. If you get X is undefined when referring to a standard API that has been available everywhere for years, it's a serious issue with your technology, regardless of its conceptual mission.

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.

That's what we've published jest-fixed-jsdom. A one-liner for people who want to focus on doing their job.

@skondrashov
Copy link

@kettanaito msw/browser import example is from the readme here: https://github.com/mswjs/msw

I did in fact use jest-fixed-jsdom, and though it got past all the polyfill errors, I just wasn't able to get the setupWorker import to work. I think the package addresses my concerns (the name could be more descriptive but that's minor), it's just that to make it work I have to reference the RTL setup docs, this thread, and several other documents across the internet along the way. I don't want to spend the time to figure out why I'm not able to import setupWorker right now but it could easily be something nobody else would run into. I'm just saying that ultimately all of these tools become useful to most people only once there is documentation to explain how to use them. It would be nice for the docs to be updated, and I'd be happy to try the steps that are laid out if you make a PR to the docs, I just don't want to jump around github and stackoverflow threads to debug this when the 2020 docs have already solved all my problems.

@kettanaito
Copy link
Contributor Author

kettanaito commented Jun 6, 2024

I just wasn't able to get the setupWorker import to work. I think the package addresses my concerns

Something is seriously mixed here. You should never use setupWorker in a Node.js process. Jest runs a Node.js process. Please use setupServer as it is advised here. I think that will resolve all your issues.

If you found that some information in MSW led you to believe you can use setupWorker in Node.js, please send it to me, I will fix it. I'm trying my best to make the environmental distinction as clear as possible. Thanks.

Edit: I'm really curious how come you are not getting runtime exceptions when using setupWorker in Node.js. Afaik, MSW will scream at you if you attempt to do that (the same is true for trying to call setupServer in the browser).

@skondrashov
Copy link

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 "setupWorker is not an export" error for both, so I never got to the point where I could get runtime exceptions. Again, I'm sure this process works if you know what you're doing, but now you're linking me to yet another doc which isn't on the RTL site, and I just want the RTL docs to tell me how to use RTL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants