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

Remove deprecated MockedRequest.body and cleanup #1318

Closed
1 task done
95th opened this issue Jul 9, 2022 · 10 comments · Fixed by #1436
Closed
1 task done

Remove deprecated MockedRequest.body and cleanup #1318

95th opened this issue Jul 9, 2022 · 10 comments · Fixed by #1436
Labels
BREAKING CHANGE Pull request introducing breaking changes. feature

Comments

@95th
Copy link
Collaborator

95th commented Jul 9, 2022

Scope

Improves an existing behavior

Compatibility

  • This is a breaking change

Feature description

Remove deprecated MockedRequest.body and cleanup after few releases with deprecation.

#1302 (comment)

@95th 95th added the feature label Jul 9, 2022
@kettanaito
Copy link
Member

Thanks for opening this one.

Yes, in the long-term, we will drop req.body entirely if we don't find a way to implement it per specification, returning a readable stream for the consumer. The difficulty here comes from the difference in handling a stream between Node and browser.

Since it'd change the expectations behind the property anyway, it may make sense to remove it first and then find a suitable approach to re-introduce it as the one that returns a stream you can hook into. Streaming request bodies is in itself a challenge, as there may be multiple handlers reading it (although never at the same time) and each would expect a fresh stream.

@kettanaito kettanaito added the BREAKING CHANGE Pull request introducing breaking changes. label Jul 9, 2022
@florianmatz
Copy link

florianmatz commented Aug 4, 2022

I did not want to start a new issues only for this; but before you drop req.bodyentirely, please consider passing on the TS Generic that is available to type the body also to the req.json method, which is currently typed with any...

Example:

interface RequestParams {
    accountNumber?: string;
    installationId?: string;
    meterId?: string;
    pin?: string;
}

rest.post<RequestParams>(`${endpoints.validation}`, async (req, res, ctx) => {
   // Nice typesafe deconstructing
   const { accountNumber, meterId, installationId, pin } = req.body;

  // "Any" is kinda uncool
  const { accountNumber, meterId, installationId, pin } =  await req.json();


}

@kettanaito
Copy link
Member

Hey, @florianmatz. Thanks for raising this concern.

Our notion behind the request body reading changes is to lift off some guesswork from the library and make you responsible for reading request bodies explicitly. This affects the types as well. I think what we will do is drop the request body generic entirely, so you could annotate the body upon reading it:

const user = await req.json() as User

Since there are multiple ways to read the same body, MSW cannot make assumptions about its type given a single generic. Think of it this way, if you annotated the body as User, then req.json() would yield you a JSON object of that shape, but req.text() would return a plain text. We can't cover that with TypeScript, and having req.text() return User type is simply incorrect.

What do you think about this direction? Do you think it's safe to hard-code certain request body reading methods' return type, while keep req.json() to infer the body type generic?

{
 text(): Promise<string>
 arrayBuffer(): Promise<ArrayBuffer>
 json(): Promise<T>
}

@florianmatz
Copy link

Hey Artem,

mmh, I think both is fair, when made clear.

I personally would prefer the option to type the json() as suggested, because I think when using text() or arrayBuffer() the return value is quite clear and completely ok to be hardcoded. Whereas json() could be any shape and is quite easy inferable...

And you don't have to use it, if someone likes to stick to a more explicit casting like as Shape (which I don't prefer, because it always looks like; nah, I just cast it anyways...)

Maybe a compromise could be something like:

json<T>(): Promise<T>

So you put your generic directly and explicitely onto the json()-Method and not onto the complete mocked request...
But maybe this is to close to the explicit casting.

@kettanaito
Copy link
Member

There's some serious dark magic with generics sometimes. I believe we're using a similar pattern right now, where we have a res.json<T>() signature but it gets inferred from the generic you give to the handler rest.get<T>(). I think we should keep JSON method annotated while hard-coding text and arrayBuffer.

Not sure how this will translate to the future .formData() to be honest. I'd expect it to act similar to URLSearchParams and never be annotated explicitly.

@nknapp
Copy link

nknapp commented Aug 11, 2022

Inspired by projects like oazapfts and openapi-client-axios, I wrote a code-generator that generates types for using msw from OpenAPI-specs. It's not public on npm right now, just part of another project, but nevertheless.

Generated from an OpenAPI spec, you can get auto-completion for the path, path-parameters, request-body props, and response-body props. It works pretty well at the moment, but it relies on the ability to specify the RequestBodyType as generic of the MockedRequest. With json<T>(): Promise<T> it would loose the ability to provide auto-completion for request-body properties.

openapi-msw-put

So please, keep some way to specify the RequestBodyType in the response-resolver.

In case you want to see the generated types...

@kettanaito
Copy link
Member

That's some outstanding tooling you're building, @nknapp!

Yes, as I've stated above, we should keep the RequestBodyType generic but apply it only to a req.json() reading method as other methods cannot be narrowed down in actuality.

That generated types thing reminds me of type generation out of GraphQL schema/queries. A powerful pattern 👍 Have you ever considered writing about it? I'd endorse your post on Twitter if you decide on it.

@nknapp
Copy link

nknapp commented Aug 12, 2022

A powerful pattern +1 Have you ever considered writing about it? I'd endorse your post on Twitter if you decide on it.

Thank you. Yes, I wanted to write about it, updated to the latest version of msw and saw that req.body is deprecated. That's how I found this issue.

Last night, I didn't notice that you are a member of msw, so I wanted to add another explained +1 to your answer.

@danbahrami
Copy link

+1 for passing down the request type generic through to the req.json() type definition. The pattern (as shown on the home page of mswjs.io) of defining the request and response body types at the top of the handler makes a lot of sense...

rest.post<LoginBody, LoginResponse>('/login', async (req, res, ctx) => {
    const { username } = await req.json()
    
    return res(
        ctx.json({
            username,
            firstName: 'John'
        })
    )
})

Because the generic is passed down to req.body you get nice type completion on req.body but nothing on req.json().

Having to typecast req.json() feels awkward because it's response is wrapped in a promise. So you have to add parenthesis to make it an expression...

// either
const { username } = (await req.json()) as LoginBody;

// or
const { username } = await (req.json() as Promise<LoginBody>);

@kettanaito
Copy link
Member

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.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
BREAKING CHANGE Pull request introducing breaking changes. feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants