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

msw@next #26

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

msw@next #26

wants to merge 2 commits into from

Conversation

dajinchu
Copy link
Contributor

@dajinchu dajinchu commented Oct 6, 2023

🎯 Changes

This makes msw-trpc work with the breaking changes in MSW 2, currently released under msw@next. I needed this because my project is on Node 18, which is not supported in msw v1, so I was forced to upgrade msw, and thus this package too.

API Change

MSW 2 changes the API a bit: https://github.com/mswjs/msw/blob/feat/standard-api/MIGRATING.md

With no context object, and response construction being done with HttpResponse.json I wasn't sure what the best design for this library would be. I have implemented a data()and getInput() function that just gets passed in the params object.

So this:

  trpcMsw.userById.query((req, res, ctx) => {
    return res(ctx.status(200), ctx.data({ id: '1', name: 'Uncle bob' }))
  }),

becomes

  trpcMsw.userById.query(({data}) => {
    return data({ id: '1', name: 'Uncle bob' })
  }),

Can also use HttpResponse from msw and type-safety still works:

  trpcMsw.userById.query(({data}) => {
    return HttpResponse.json({results: {data: { id: '1', name: 'Uncle bob' }}})
  }),

Other API proposals:

  // Extend the HttpResponse prototype (but now how will we apply superjson? clone the response and rewrite it?:
  trpcMsw.userById.query(({data}) => {
    return HttpResponse.trpc({ id: '1', name: 'Uncle bob' })
  })

  // do raw data (how will you return an error):
  trpcMsw.userById.query(({data}) => {
    return { id: '1', name: 'Uncle bob' }
  })

Would love some feedback on the API! Thanks.

Comment on lines +82 to 92
test('throwing error works', async () => {
server.use(
mswTrpc.userById.query(() => {
throw new TRPCError({ code: 'BAD_REQUEST' })
})
)
await expect(async () => {
await trpc.userById.query('1')
}).rejects.toThrow(new TRPCClientError('BAD_REQUEST'))
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the API for returning 404, to throw a NOT_FOUND TRPCError?

@maloguertin
Copy link
Owner

Hey thanks for this! I'll try to review this tomorrow. Sorry I have been pretty busy lately.

@dajinchu
Copy link
Contributor Author

dajinchu commented Dec 7, 2023

No worries. Latest commit actually changes it to the "raw data" option and catches TRPCError. Been in use at my company since without issue

@danilofuchs
Copy link
Contributor

I believe the PR must be updated to depend on [email protected]

@@ -43,35 +44,35 @@ const createUntypedTRPCMsw = (
{},
{
get(_target: unknown, procedureKey) {
if (procedureKey === 'query') {
if (procedureKey === 'query' || procedureKey === 'mutation') {
Copy link
Owner

Choose a reason for hiding this comment

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

What would you think of creating an helper for this to clean up the code a bit:

const httpMethodByProcedureKey = {
  query: http.get,
  post: http.post
}

const getInputByProcedureKey = {
  query: getQueryInput,
  mutation: getMutationInput
}

const createHandler(procedureKey: 'query' | 'mutation') => {
  const httpMethod = httpMethodByProcedureKey[procedureKey]
  const getInput = getInputByProcedureKey[procedureKey]
   
   return handler => (httpMethod)(
              buildUrlFromPathParts(pathParts),
              async (params): Promise<any> => {
                try {
                  const body = await handler(await getInput(params.request, transformer))
                  return HttpResponse.json({ result: { data: transformer.input.serialize(body) } })
                } catch (e) {
                  if (e instanceof TRPCError) {
                    const status = getHTTPStatusCodeFromError(e)
                    return HttpResponse.json(
                      {
                        error: {
                          message: e.message,
                          code: TRPC_ERROR_CODES_BY_KEY[e.code],
                          data: { code: e.code, httpStatus: status },
                        },
                      },
                      { status }
                    )
                  } else {
                    throw e
                  }
                }
              }
            )
}

@maloguertin
Copy link
Owner

I believe the PR must be updated to depend on [email protected]

Do you mean the PR in itself or the package dependencies?

@maloguertin
Copy link
Owner

This PR seems to adress this feature request: #13

@danilofuchs
Copy link
Contributor

danilofuchs commented Dec 8, 2023

I believe the PR must be updated to depend on [email protected]

Do you mean the PR in itself or the package dependencies?

The package dependencies must be changed from msw@^1.0.1 to msw@^2.0.10. This PR currently sets it to msw@next.

Of course, that would be a breaking change. (Also for dropping support for Node <18)

@maloguertin
Copy link
Owner

I believe the PR must be updated to depend on [email protected]

Do you mean the PR in itself or the package dependencies?

The package dependencies must be changed from msw@^1.0.1 to msw@^2.0.10. This PR currently sets it to msw@next.

Of course, that would be a breaking change. (Also for dropping support for Node <18)

Oh you're right, I thought I had seen msw@next on their website

"zod": "^3.20.2"
},
"peerDependencies": {
"@trpc/server": ">=10.9.0",
"msw": ">=0.49.3"
"msw": "next"
Copy link
Owner

Choose a reason for hiding this comment

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

Set dependency to ^2.0.0

@maloguertin
Copy link
Owner

We also need to update the doc

@madi-tracksuit
Copy link
Contributor

Anything I can do to help unblock and get this shipped? Our team also has a prerequisite for msw v2.

If it's just documentation or a breaking change hesitation could it be shipped as a pre-release version?

@maloguertin
Copy link
Owner

Anything I can do to help unblock and get this shipped? Our team also has a prerequisite for msw v2.

If it's just documentation or a breaking change hesitation could it be shipped as a pre-release version?

I wouldn't mind that, I'm not sure I'll have the time this week but I'm open to review the PR for that!

@madi-tracksuit
Copy link
Contributor

For publishing a pre-release version? Would need access to npm for that, but I'll happily bump the package.json

@maloguertin
Copy link
Owner

For publishing a pre-release version? Would need access to npm for that, but I'll happily bump the package.json

yes sorry that is what I was referring to!

@madi-tracksuit
Copy link
Contributor

@maloguertin #31

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.

4 participants