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

docs(example): Remix + MSW example #2156

Merged
merged 9 commits into from
Mar 9, 2022
Merged

docs(example): Remix + MSW example #2156

merged 9 commits into from
Mar 9, 2022

Conversation

Girish21
Copy link
Contributor

No description provided.

@MichaelDeBoey
Copy link
Member

CC/ @kettanaito


## Example

If the external APIs are metered and charged for the number of API calls made, it's pretty easy to burst through the API quota during development. Also, we need an active network during development. For some, this may be an issue. So instead, we can mock the external API using MSW, which intercepts the API calls from the server and returns a mocked response.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are some great points!

When I was researching the applications of API mocking, having the ability to develop against a non-existing backend was one of the most common ones. This also applies to situations when you have a backend but it's not in the state that you need (you can augment responses with mocking as well).

API mocking is also powerful for debugging, as you can reliably reproduce any HTTP call by mocking it.

Perhaps, consider some of these points. You can read more about them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sure, I will reword here

@@ -0,0 +1,25 @@
# Remix + MSW

This example demonstrates [MSW's][msw] usage with Remix to mock external APIs called from the Remix server during development.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a nitpick but the word "external" may make an impression that MSW is only for third-party API mocking. I think you're safer saying just "to mock any HTTP calls" (I'd even omit API as it can mean a bunch of things depending on the context).


## Relevant files

- [mocks](./mocks/index.js)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is neat! I like having sections like this one for people to quickly see where the relevant setup is located. One thing I may suggest is to also add a brief description of what a particular file achieves (i.e. "describes how to intercept requests", or "registers the service worker").

const { rest } = require("msw");
const { setupServer } = require("msw/node");

const handlers = [

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I highly recommend moving handlers to a separate file. This way both browser and Node.js integrations can use the same handlers. At the moment that's not possible: you import msw/node which will fail terribly in the browser as it relies on Node.js built-ins (thus the name).

Take a look at our setup from examples. This has proven to be quite a great default setup (I even think Kent was one of the first people to introduce it).

console.info("MSW initialised");

process.once("SIGINT", () => server.close());
process.once("SIGTERM", () => server.close());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair, if the process is terminated there's no way the server would keep doing anything. Keep in mind that despite the name, there are no actual HTTP servers establishes. setupServer is a plain function that listens to intercepted events and handles them.

Perhaps you had some other scenarios in mind for these process events?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kentcdodds, what do you think about adding the listeners?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never done it and I don't know what situation would necessitate this. I think it's unnecessary

Copy link

@kettanaito kettanaito Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setup would make sense if server spawned a child process or any other side-effect detached from the current process' thread that would have to be managed manually. That is not the case, so if the process exists/terminates it will also destroy the execution context for server.

You can safely remove this.

console.info("MSW initialised");

process.once("SIGINT", () => server.close());
process.once("SIGTERM", () => server.close());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this module is used anywhere yet, but we need to take Live Reload into account. We need to ensure that setupServer is not called repeatedly, as that will lead to event emitter leaks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that it will not be because we're using -r to require this file 👍

@kettanaito
Copy link

This is definitely a good start!

I think the example is yet to add a browser integration (setupWorker) and figure out the right places to establish them in Remix. I'd suspect that adding a Node.js integration to root.jsx may be the case (watch out for Live Reload), not sure where the worker registration would go.

@rodrigofuentes
Copy link

@kettanaito @Girish21 It seems that headers aren't being read properly during unhandled requests.

[MSW] Expected response resolver to return a mocked response Object, but got undefined. The original response is going to be used instead.

...

Response {
  size: 0,
  timeout: 0,
  [Symbol(Body internals)]: {
    body: PassThrough {
      _readableState: [ReadableState],
      _events: [Object: null prototype],
      _eventsCount: 5,
      _maxListeners: undefined,
      _writableState: [WritableState],
      allowHalfOpen: true,
      [Symbol(kCapture)]: false,
      [Symbol(kCallback)]: null
    },
    disturbed: false,
    error: null
  },
  [Symbol(Response internals)]: {
    url: 'https://randomuser.me/api',
    status: 200,
    statusText: 'OK',
    headers: Headers { [Symbol(map)]: [Object: null prototype] },
    counter: 0
  }
}

in interceptors: ClientRequest/utils/getIncomingMessageBody.ts and ClientRequest/NodeClientRequest.ts

@rodrigofuentes
Copy link

My other comment is irrelevant, Node version issue.

mswjs/msw#1125 (comment)

@kentcdodds kentcdodds self-assigned this Mar 3, 2022
@kentcdodds
Copy link
Member

We may need to either downgrade MSW in this example or wait until this issue is resolved: mswjs/msw#1120

@edmundhung
Copy link
Contributor

It might be worth mentioning that MSW current does not support intercepting server requests on Cloudflare Workers / Pages which uses undici instead of typical http based implementation.

Ref: mswjs/interceptors#159

@MichaelDeBoey
Copy link
Member

msw@0.39.0 is released.

https://github.com/mswjs/msw/releases/tag/v0.39.0

@kentcdodds
Copy link
Member

I say we upgrade, but it may be good to add a note that v0.39.0 requires Node v16. AWS currently does not support Node v16 (😭) so if you want to use the latest MSW and you deploy to AWS, you'll be running a different version of Node locally from the one that runs in production 😬

@Girish21
Copy link
Contributor Author

Girish21 commented Mar 7, 2022

Wait. MSW in production? what would be the use case for that? 😅

@Girish21
Copy link
Contributor Author

Girish21 commented Mar 7, 2022

This is definitely a good start!

I think the example is yet to add a browser integration (setupWorker) and figure out the right places to establish them in Remix. I'd suspect that adding a Node.js integration to root.jsx may be the case (watch out for Live Reload), not sure where the worker registration would go.

Regarding adding a browser integration, @kentcdodds, your thoughts?

@kentcdodds
Copy link
Member

Wait. MSW in production? what would be the use case for that? 😅

That's not what I meant. What I meant is that if you are deploying to AWS but you also want to use the latest version of MSW then locally you're using Node 16 but in production you're using Node 14. This is not advisable. So if you're deploying to AWS then you can't use the latest version of MSW.

Regarding adding a browser integration

In my experience, it doesn't make much sense to use MSW in the browser with Remix because all your requests happen on the server via loaders/actions. You don't want to get in the business of mocking out the requests that Remix makes to loaders and actions. Instead it's much easier to mock out the third party services your server is calling. So I wouldn't bother with browser integration here. MSW + Remix is really quite simple :)

@kentcdodds kentcdodds added the needs-response We need a response from the original author about this issue/PR label Mar 7, 2022
@kettanaito
Copy link

Hey, folks. There might have been confusion on my side when it comes to Node.js versions support. Allow me to clarify below.

MSW has indeed migrated to Node.js v16.14.0 (read v16). This means that our packages use the Node.js version to install, build, and test themselves. We always guarantee the Node.js version on which we are developing. This does not mean, however, that MSW won't run on older versions of Node.js!

Some have experienced errors when installing msw in projects with older versions of Node.js. I connect those to Yarn specifically and how Yarn handles version mismatches in the engines property of package.json. That property is meant to produce warnings by design, and rightfully does so when installing msw via NPM. Yarn 1.x does not seem to respect that specification, producing an error, exiting the installation process. I wonder if that's the case for Yarn 2.x.

To summarize:

  • We've moved towards officially supporting only Node.js v16+;
  • MSW can still run on older versions of Node.js (in fact, I think @MichaelDeBoey has confirmed it runs without issues on Node.js v12);
  • I do believe we're using the engines property correctly, and it comes down to the package manager you're using to how it interprets that property.

I'm open to suggestions if you think this isn't the right way to handle such migration.

@kettanaito
Copy link

@rodrigofuentes, I think there's something wrong with the mocked response in that headers scenario (the error message above suggests that—nothing was returned from the handler). The headers appear to be in order in your code snippet. Note that Headers never store values and instead use prototype getters/setters to read/write them respectively. That's why you only see what appears an empty object when logging it out.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! Just one last thing and then we can get this merged. Thanks a ton for iterating on this so much! It's great 👏👏


## Gotchas

MSW currently does not support intercepting requests made by `undici`. Since Cloudflare Workers and Pages implements `fetch` using `undici`, we can't use MSW to mock the HTTP calls. You can follow this issue [#159](https://github.com/mswjs/interceptors/issues/159) to track the progress.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing it's miniflare that's the limitation here because you don't actually run cloudflare workers locally. Might be good to call that out specifically.

Copy link
Contributor

@edmundhung edmundhung Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The limitation comes from miniflare. But Cloudflare Pages users might not be aware of miniflare as it is an internal dependency of wranlger. 😅

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stellar 👏👏 Thanks a lot!

@kentcdodds kentcdodds merged commit 67df1c9 into remix-run:main Mar 9, 2022
jstafman pushed a commit to jstafman/remix that referenced this pull request Mar 18, 2022
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
@MichaelDeBoey MichaelDeBoey added examples and removed needs-response We need a response from the original author about this issue/PR labels Mar 22, 2022
@wingy3181
Copy link

wingy3181 commented Apr 7, 2022

Stupid question about this example or maybe something to add to the README is how to toggle MSW?

I got it working by import '../mocks' in the root.tsx file but is this the intended technique for this example?
(obviously not the best since its not separating prod vs dev)

Was comparing it to kent's setup on his website (https://github.com/kentcdodds/kentcdodds.com/blob/bb3a2839c236bd11702e9fc11bd696cd98ac7bc3/package.json#L35) and what he has talked about in his talks....... but it's a different setup...

Not sure if i should be asking this on discussions or not....or this closed PR
but will try here first

@jmaldon1
Copy link

jmaldon1 commented May 1, 2022

@wingy3181 This is how I ended up doing it. (Not sure why this isn't in the docs anywhere. Maybe this is wrong?)

// app/entry.server.tsx
...
if (process.env.NODE_ENV === "development") {
  const server = require("../mocks");

  server.listen({ onUnhandledRequest: "warn" });
  console.info("MSW initialised");
}
...
// mocks/index.ts
import { setupServer } from "msw/node";
import { handlers } from "./handlers";

// This configures a request mocking server with the given request handlers.
const server = setupServer(...handlers);

module.exports = server;

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

Successfully merging this pull request may close these issues.

8 participants