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

fix: annotate the "requestId" response resolver argument #1969

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

skvale
Copy link
Contributor

@skvale skvale commented Jan 15, 2024

After trying out #1942 I found the types weren't working as expected even though requestId was passing through

Before

Screenshot 2024-01-15 at 8 07 00 AM Screenshot 2024-01-15 at 8 06 46 AM

After

Screenshot 2024-01-15 at 8 04 21 AM

@kettanaito kettanaito changed the title fix: type for ResponseResolverInfo includes requestId fix: include "requestId in the type for ResponseResolverInfo Jan 15, 2024
@kettanaito kettanaito changed the title fix: include "requestId in the type for ResponseResolverInfo fix: include "requestId in the "ResponseResolverInfo " type Jan 15, 2024
@@ -736,7 +736,8 @@ describe('run', () => {
userId: 'abc-123',
},
})
const result = await handler.run({ request })
const requestId = 'requestId'
Copy link
Member

Choose a reason for hiding this comment

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

Let's use either the uuid4() we already have or crypto.randomUUID() here. It's a little touch but the more your tests resemble the way your system works, the better.

@skvale skvale changed the title fix: include "requestId in the "ResponseResolverInfo " type fix: include "requestId" in the "ResponseResolverInfo" type Jan 16, 2024
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

Fantastic! Thank you for yet another contribution, @skvale 👏

I've added the type tests for the requestId argument of the response resolver so we could've caught this issue sooner (no tests failed === tests were missing).

@kettanaito kettanaito changed the title fix: include "requestId" in the "ResponseResolverInfo" type fix: annotate the "requestId" response resolver argument Jan 16, 2024
@kettanaito kettanaito merged commit f22294e into mswjs:main Jan 16, 2024
10 checks passed
@kettanaito
Copy link
Member

Released: v2.1.1 🎉

This has been released in v2.1.1!

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.

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.

None yet

3 participants