-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
Adopt Fetch API primitives #1404
Comments
Thanks for writing this down, @kentcdodds. I've been thinking about using native Fetch's |
First of all, I think this is a good change. Embracing the platform is something MSW has been doing almost since the beginning, both when using the Service Worker API as well as operating with Fetch In a very drafty fashion, this is the public API I imagine: // handlers.js
import { rest } from 'msw'
export const handlers = [
rest.get('/user', () => {
const user = JSON.stringify({ name: 'John' })
return new Response(user)
})
] Now, let’s talk about some concerns with such API change. Insufficient dataBoth
The fetch API also lacks certain convenience features such as responding with a JSON body quickly. I think it’s too verbose to be consumed directly. Environment supportMSW still supports versions of Node.js that have no native Even outside of support, Node.js handles requests and responses differently based on the request issuing actor:
This difference was our main motivator to introduce Next, let’s talk about how we can mitigate some of my concerns above. Similar to Remix, MSW can provide additional request information like path parameters or cookies alongside the request instance instead of embedded in it: // This is an approximate API that reflects that of Remix.
rest.get('/resource/:id', ({ request, params, cookies }) => {
console.log(params.id)
}) I suppose we can help with faster response creation in a similar way: rest.post('/user', ({ request, json }) => {
return json({ id: 'Name' })
}) But with such API I’d be careful with what this whole object represents. If we stuff it with all sorts of utilities, it will become hard to use. I also like the functional approach the current API has as you can abstract specific parts of the response to functions and reuse them. This surfaces rather well if we decide to compose multiple response data at once: rest.get('/resource', ({ json, set, delay }) => {
// return ???
}) This brings us to the point that there are 3 kinds of API we should expose:
Okay, what is that “network record” thingie? I’d describe it as the response representation in the “Network” tab. In the addition to the In this regard, the resolver function should really return an instruction on how to respond, and response is just a part of that instruction. import { json } from 'msw'
rest.get('/resource', ({ request }, respondWith) => {
return respondWith(
new Response('hello'), // or "json()"
{ timing: 1200 }
)
})
As I think about this API, the composability concern becomes bigger. The response/network record separation feels natural but we need to design it in a way that would allow people to abstract, compose, and reuse certain response/network behaviors with ease. API draftTo conclude this initial post, this is somewhat the API to consider: import { compose, json, status, NetworkEntry } from 'msw'
export const handlers = [
rest.get('/resource', ({ request }) => {
// Returning a plain fetch Response.
return new Response('hello world')
}),
rest.post('/user/:id', ({ request, params }) => {
const { id } = params
// Returning a plain Response but using built-in
// utilities to compose it more efficiently.
return json({ id })
}),
rest.post('/movie', async ({ request }) => {
// Reading request per fetch API.
const movie = await request.json()
// Composing multiple response utilities
// into a single "Response" instance.
const response = compose(
status(201),
json(movie),
)
// Responding with a network record (entry)
// to control response metadata like timing.
return new NetworkEntry(response, { timing: 1200 })
})
] I’d much appreciate a discussion on this so we could find the right API together. |
While this is a breaking change, there are a couple of things I'd like to keep:
|
Personally, I'd strongly advise with sticking with the standard objects and use utility functions to help with common use cases. Otherwise you violate the principle of least surprise leading to confusion on the part of users. It's almost worse to make it look like a standard object with slight changes because people will assume that it's standard and when things don't work the same they may get confused. It also means that code isn't as portable. A firm stance on standards is the approach that I took with Testing Library and it was very successful for both the library as well as the users. Same goes for Remix. Having to construct |
The API you've shown works well for me. I think it's ok that a handler could return a |
We could even expose the request URL, pre wrapped as an additional argument to that callback, if necessary ,which would keep the request object native, but avoid needing to wrap it in new URL manually - although I'd agree it seems like it isn't unreasonable to wrap those where necessary instead of on every request |
Yup. I just think it's an important imperative to keep things as standard as possible rather than almost standard. Even in the name of convenience. |
@kentcdodds, agree on the adhering to standards point.
As practice shows that headers are not the best place to persist library implementation details. We can try that route, as we're speaking of response header in this context, but I'd leave room open for better API. @mattcosta7, yeah. That's the route I'd take with
In this regard, I'd encourage them to create a new // This is a response patching scenario with extra steps.
rest.get('/resource', async ({ request }) => {
const proxyRequest = new Request(request)
proxyRequest.headers.delete('Some-Header') // is this permitted?
const originalResponse = await fetch(proxyRequest)
return new Response(...)
}) |
One of the things I wanted to do for some time is separate mocked response from the resolver instructions (see #1207). Mocked response is not the only thing that the resolver may return (i.e. may also return a network error). I think it makes sense for the resolver to always return an instruction (internally):
This is not a user-facing API but rather how I'd like to implement things under the hood. Something like a set of possible instructions with their data: type ResolverInstruction =
{ type: 'USE_MOCK', mockedResponse: Response } |
{ type: 'USER_NETWORK_ERROR', networkError: NetworkError } |
{ type: 'PASSTHROUGH } Separating instructions (intentions) from the mocked payload should bring clarity to the API, which may eventually make the public API easier as well. For example, there's no way to respond with a network error once at the moment (#413). It's because we couple the "once" capabilities into the response composition ( |
One of the great side-effects on adhering to the spec is that we could then minimize issues like #1327. |
Using Fetch API
|
I like this conceptually. Exposing helps to build ergonomic responses seems like a good way to go (and I believe remix does similar for loaders?) I'm not sure we'd need to have the class construct, instead of just the methods themselves? |
Yeah, the class construct is pseudocode. I ended up having
Yes! Remix exposes |
Do you think convenience shorthands have a place to be? Something like |
One thing I find useful (among others) when working with MSW is the ability to provide types for request and response, like this: rest.get<
RequestBody,
PathParams,
ResponseBody
>(
URL,
(request, response, context) => {
// Some code.
}
) How would it look like when the changes are applied? Do |
IMO, from the library's perspective, it would be beneficial to use the platform under the hood. But from the consumer's perspective, a convenient API would be more useful rather than a compliant one. It is the same as using |
@vasilii-kovalev I disagree. The benefit of using the platform here is compatibility with other tools and knowledge sharing. If the API is "just the platform" then people who know the platform already know the API. If they don't know the platform, they'll learn it as they learn the API which is transferable knowledge for them which is a big win. As for the namespace object, I don't like it because it is unnecessarily verbose and forces me to first auto-complete the object, and then auto-complete the method I want. If instead these were simple methods exposed by a module within MSW, I could have the option of importing them as a namespace if I wanted to, but I wouldn't have to do that. The fact that people get to choose the name of the namespace import (so there's no enforced consistency) is not a big deal in my mind. |
@kentcdodds, I suppose we just have different preferences 🙂 I don't have any problems with libraries/frameworks introducing a new API/concepts, if they solve the problem I have, speed up my work and keep the code's understanding/correctness at an acceptable level (= gets along with TypeScript in this case). React, TypeScript, MSW and other different libraries' API were new for me once, but I used to it. When talking about that namespace vs. handler function's parameter options, I only care about types connection between different parts of the handler. If it will be possible to add types separately for |
That was kind of the same thinking I was trying to express earlier
// 'msw/response'
function toResponseInit(init: ResponseInit | number): ResponseInit {
if (typeof responseInit === "number") {
return { status: responseInit } satisfies ResponseInit
}
return init
}
// I don't think we actually have a better way to type this here, since techically a `jsonable` is anything that implements a `toJSON` method, or anything that's type Scalar = string|null|undefined|number|boolean type JSON = Scalar | Record<string, scalar> | Array<JSON> which is effectively almost anything anway?
export function json(body: any, init: ResponseInit | number = 200): Response {
const response = new Response(JSON.stringify(body), toResponseInit(init))
response.headers.set('Content-Type', 'application/json; charset=utf-8')
return response
}
export function text(body: string|null, init: ResponseInit | number = 200): Response {
return new Response(body, toResponseInit(init))
}
export function formData(body: FormData|null, init: ResponseInit | number = 200): Response {
return new Response(body, toResponseInit(init))
}
export function arrayBuffer(body: ArrayBuffer|null, init: ResponseInit | number = 200): Response {
return new Response(body, toResponseInit(init))
}
export function redirect(url: string, init: ResponseInit | number = 302): Response {
const responseInit = toResponseInit(init)
if (!responseInit.status) {
responseInit.status = 302;
}
const response = new Response(null, responseInit);
response.headers.set("Location", url);
return response
};
// in mocks/handlers (or wherever)
import {json, formData, arrayBuffer, redirect, text} from 'msw/response' these also should benefit from a little treeshaking (even though it's very light wrappers) where a single object would not allow that (in addition to the easier editor inference). I'm not sure what the path to type-safety in practice here is, since response objects aren't generic - although given the mention earlier, for instance, that |
Shipping fewer library-specific abstractions is, generally, a sign of good API design. It means a much leaner learning curve for anyone wishing to interact with the API. That's why I want to migrate to Fetch API primitives internally to the fullest extent. But we cannot ignore the fact that the platform is often lacking behind. It's the reason why we have tools like Babel, PostCSS, and many others. And API mocking is one of the areas where the web standards offer no solution at all. You're simply not meant to declare responses in your code, only receive and handle their representations ( In this light, I think the best direction to go is a mix of the two:
But also:
So, to summarise:
On
|
@mattcosta7, thanks for writing that up! I'm sharing some of my concerns about structuring API around the assumption of wildcard imports above. That's not a good idea, imo.
Not really applicable for a devtool, which MSW is.
You're right, Fetch API primitives are not type-safe. But we can make |
Proposal: replace
|
Maybe not really necessary for a dev tool, and these are all generally small, but fewer bytes of code shipped to the browser does shave little bits of time here/there for things like playwright tests (where the scripts are fetched/parsed/evaled for every test) I'm not sure I see the value in adding a lot of extra text to namespace them all in an object, but I don't feel strongly on that, so happy to try it and see how it feels too
Of course we can always do something like export const HttpResponse = {
json<T>(body: T, init: ResponseInit | number = 200): Response {
const response = new Response(JSON.stringify(body), toResponseInit(init))
response.headers.set('Content-Type', 'application/json; charset=utf-8')
return response
},
...otherMethods
} Where rest.get<
RequestBody,
PathParams,
ResponseType
>(url, (request, HttpResponse) => {
const body = {} satisfies ResponseType
return HttpResponse.json<ResponseType>(body)
}) One thing tough with typings here are that a Importing const rest = {
get(
url: string,
// keeping the resolve
responseResolver: (
// maybe arguments here look more like request/params. maybe not. the resolver function and context objects propably make less sense here in that case? (also outside the scope of this change for now, but just for discussion)
request: Request,
// maybe determinable from the url string directly with path parameter extraction types like https://github.com/ghoullier/awesome-template-literal-types#router-params-parsing
params
) => Response) {...}
}
rest.get<
// with http requests, json can be munged, by providing a custom Request instead of a fetch native request. there's no guarantee the request actually gets _made_ with that type, so maybe better to _not_ type it directly and rely on
RequestBody,
// json could be handled like below, but so would incorrect types
ResponseType
>(url, (request) => {
const body = {} satisfies ResponseType
const res = HttpResponse.json<ResponseType>(body)
// works
return res
return new Response(null) // also works (assuming the previous return isn't there
}) Since the return here is actually a Overriding types for Response here could be doable |
Yep. And that's why we will generally recommend using |
I yield on the namespace thing. It's not that important to me. I stand very firm on the web standards thing though. Please do not use anything other than web standard Request/Response, otherwise the entire goal of the original issue is left unsatisfied for reasons I've already explained. Also, I'm a fan of |
@kentcdodds, agree. Yes, as I've said, with this change we will fully support Fetch API primitives: the |
Perfect ❤️ |
Looks like the |
Actually importing from 'msw/response' it's the context that you are talking about so there's no need to have an object. I'm thinking in these functions/methods as just utilities that you can use in order to get a |
At this point it's decided that
Technically, you can. We will support this and you can use global Response anytime you want. But in practice, if you wish to write isomorphic handlers (a fancy term I will adapt that means "handlers that can run in both browser and Node"), you will be adviced to use either a import { rest, Response, FormData } from 'msw'
// This handler can be executed in both browser and Node.js
// due to the mirror exports of "Response" and "FormData".
rest.get('/user', () => {
const data = new FormData()
data.set('name', 'Alice')
return new Response(data)
}) |
Amazing, let me know if I can help with something even if it's just docs. I would like to help with this. |
@dbritto-dev, I know I could certainly use your help! Here are a couple of ways you can help me make this change a reality faster:
|
@kettanaito cool, I'll work on that |
Edit: Deleted my comment as I see it's already been discussed here |
Released: v2.0.0 🎉This has been released in v2.0.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Scope
Improves an existing behavior
Compatibility
Feature description
I think this could be done in a way that is non-breaking. But I think it'd be great to be able to write my handlers similar to how I write my Remix loaders/actions. They receive an object with
{ request, params, context }
and return aResponse
. Could even use the Remix ponyfill for Request/Response: https://github.com/remix-run/web-std-ioThe text was updated successfully, but these errors were encountered: