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

feat: add "requestId" as response resolver argument #1942

Merged
merged 6 commits into from
Jan 15, 2024
Merged

Conversation

skvale
Copy link
Contributor

@skvale skvale commented Dec 27, 2023

Fixes: #1933

In MSW v1 I was able to access req.id in a handler function which corresponded with the requestId in events.

For example

worker.events.on('response:mocked', ({requestId}) => {
// and
rest.post('my-url', (req) => { req.id 

were the same value

In v2, the request object doesn't have an id attribute. From what I can tell the requestId is unique to events

This seems like a nice to have addition to the handler args that wouldn't be hard to add

@kettanaito
Copy link
Member

Thanks for submitting this so quickly, @skvale! I will come back to review this in January.

@kettanaito kettanaito changed the title feat: pass requestId to handlers feat: add "requestId" as response resolver argument Jan 5, 2024
@@ -216,6 +217,7 @@ export abstract class RequestHandler<
public async run(args: {
request: StrictRequest<any>
resolutionContext?: ResponseResolutionContext
requestId?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be required instead of optional? are we leaving it optional to avoid making it required for non-standard usage? just curious if we can accidentally miss this if it's optional?

Copy link
Contributor Author

@skvale skvale Jan 5, 2024

Choose a reason for hiding this comment

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

I'm wondering if this is considered exposed of it's OK to change this function signature to get the required arg before the optional one

So from this

/**
 * Returns a mocked response for a given request using following request handlers.
 */
export const getResponse = async <Handler extends Array<RequestHandler>>(
  request: Request,
  handlers: Handler,
  resolutionContext?: ResponseResolutionContext,
  requestId?: string,
): Promise<ResponseLookupResult | null> => {

to this

/**
 * Returns a mocked response for a given request using following request handlers.
 */
export const getResponse = async <Handler extends Array<RequestHandler>>(
  request: Request,
  handlers: Handler,
  requestId: string,
  resolutionContext?: ResponseResolutionContext,
): Promise<ResponseLookupResult | null> => {

Copy link
Member

Choose a reason for hiding this comment

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

@skvale, I believe your last question is resolved since the run() method now accepts a single object argument where the argument position is irrelevant. Thanks for updating the PR!

resolutionContext,
}: {
request: Request
requestId: string
Copy link
Member

Choose a reason for hiding this comment

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

The only thing I'm not sure about here is whether requiring requestId for the .run() method is right.

While this is not widely used, you can call a request handler directly using its .run() method. Just accepting request makes sense. Demanding requestId means the consumer has to contrive that ID. Hmm.

I think we can make this required and suggest using crypto.randomUUID() as a placeholder value if someone has to call a handler directly. If you want to—you must provide a request ID.

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.

This looks fantastic! Great work, @skvale 👏

@kettanaito kettanaito merged commit 51ab8cc into mswjs:main Jan 15, 2024
10 checks passed
@kettanaito
Copy link
Member

Welcome to contributors, @skvale! 🎉

@kettanaito
Copy link
Member

Released: v2.1.0 🎉

This has been released in v2.1.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.

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.

Pass "requestId" as a response resolver argument
4 participants