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

Supports async response transformers #467

Merged
merged 7 commits into from Nov 29, 2020

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented Nov 20, 2020

Changes

  • Now it's possible to create async response transformers.
  • Exports compose function publicly to compose multiple standard response transformers as a part of a custom response transformer.
  • Adds browser integration tests for an async response transformer.
  • Handles response calls as potentially async in unit tests.
  • Aligns unit tests to be shallow (drops describe blocks).

GitHub

Roadmap

  • Annotate compose properly (infer function type, set return type)


const worker = setupWorker(
rest.get('/image', async (req, res, ctx) => {
return res(ctx.status(201), await jpeg(base64Image))
Copy link
Member Author

Choose a reason for hiding this comment

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

I find it extremely readable to await the transformer upon invocation, as opposed to drilling asynchronicity all the way down to the response composition tree.

import { ResponseTransformer, setupWorker, rest, context, compose } from 'msw'
import base64Image from 'url-loader!../../fixtures/image.jpg'

async function jpeg(base64: string): Promise<ResponseTransformer> {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an example of a custom jpeg response transformer that appends a given Base64 image to the mocked response. It utilizes newly exported compose function, as well as the ability to make the transformer async.

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.

Don't we have to await somewhere (I think in compose)?

My thoughts:
ResponseTransformer can now be a promise and is a parameter (defaultTransformers) for createResponseComposition. This parameter is used to build the resolvedTransformers, which is passed to the compose method. So, when we're in compose, we still have the promise, and we're not awaiting its result.

(I could be wrong tho, I didn't test it)

@kettanaito
Copy link
Member Author

Thanks for the feedback, @timdeschryver.

Don't we have to await somewhere (I think in compose)?

Not with the current implementation. We allow response transformer to be async, but it must be awaited within the response resolver (which is already supported as async). That way the list of transformers that we supply to compose consists of already resolved transformer functions and there's no need to await in the compose itself:

async (req, res, ctx) => {
  // `res` will get the resolved value of `customTransformer`, not the Promise
  return res(await customTransformer())
}

ResponseTransformer can now be a promise and is a parameter (defaultTransformers) for createResponseComposition. This parameter is used to build the resolvedTransformers, which is passed to the compose method.

That's a great point. I'll add a test that ensures that async transformer passed to the custom response composition function still executes properly 👍

@kettanaito kettanaito force-pushed the 457-async-response-transformers branch from 19ece18 to 10a1727 Compare November 21, 2020 09:12
@kettanaito
Copy link
Member Author

kettanaito commented Nov 21, 2020

What do you think about this extension to the compose function?

export function compose(...funcs: Array<(...args: any[]) => any>) {
return funcs.reduce((f, g) => {
return (...args: any[]) => {
const res = g(...args)
if (res instanceof Promise) {
return Promise.resolve(res).then(f)
}
return f(res)
}
})
}

I didn't want to make it always return a Promise, which would ruin a synchronous execution. At the same time if any of the functions returns a Promise the compose returns a Promise accordingly.

I only wish we could still annotate compose so it understands that return signature.

I've also added an integration test for an async response transformer as the value of defaultTransformers of a custom response.

timdeschryver
timdeschryver previously approved these changes Nov 21, 2020
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.

LGTM, nice @kettanaito!

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 27, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5ec66d2:

Sandbox Source
MSW React Configuration

@kettanaito kettanaito force-pushed the 457-async-response-transformers branch from 4a39def to 5ec66d2 Compare November 27, 2020 09:40
@kettanaito
Copy link
Member Author

Experiencing some TS issues during the compilation of an Angular project as a part of smoke tests.

@timdeschryver
Copy link
Member

@kettanaito this could be due to miss matching typescript versions.
I'm going to upgrade the angular example to the new angular version, which also supports TS 4.x.

@kettanaito
Copy link
Member Author

This is amazing, @timdeschryver! Thank you for figuring that out. I'll review your Angular pull request.

@timdeschryver
Copy link
Member

No problem! It was an error that I've had seen before 😅

@kettanaito kettanaito merged commit f91c850 into master Nov 29, 2020
@kettanaito kettanaito deleted the 457-async-response-transformers branch November 29, 2020 12:48
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.

Support asynchronous response transformers
2 participants