-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
Conversation
Thanks for submitting this so quickly, @skvale! I will come back to review this in January. |
src/core/handlers/RequestHandler.ts
Outdated
@@ -216,6 +217,7 @@ export abstract class RequestHandler< | |||
public async run(args: { | |||
request: StrictRequest<any> | |||
resolutionContext?: ResponseResolutionContext | |||
requestId?: string |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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> => {
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 👏
Welcome to contributors, @skvale! 🎉 |
Released: v2.1.0 🎉This has been released in v2.1.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
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
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