-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
api-fetch: Type several of the middlewares #29719
Conversation
61bf58f
to
ea308f8
Compare
Size Change: -1 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
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.
Looks good, thank you 👍
* @param {import('../types').ApiFetchRequestProps} options | ||
* @param {(options: import('../types').ApiFetchRequestProps) => import('../types').ApiFetchRequestProps} next | ||
* @return {import('../types').ApiFetchRequestProps} The enhanced request. | ||
* @type {import('../types').ApiFetchMiddleware & { nonce: 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.
I guess we could do a bit better with typing the { nonce: string }
part.
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.
What would you suggest? This is the only way I know how to type a function with extra properties on it other than using a new interface:
interface ApiFetchNonceMiddleware extends ApiFetchMiddleware {
nonce: string;
}
but that's effectively the same as what I have inline here.
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 agree, that's not much better 👍 Let's keep as-is for now
Thanks for the review @tyxla! |
Description
Adds type checking to the following middleware:
It also simplifies the types used in the
nonce
middleware.It also corrects the return types for the
ApiFetchMiddleware
and itsnext
function. These eventually always return aPromise
as the final middleware eventually calls to the default handler. Each middleware itself can also return a Promise instead of callingnext
(seepreloading
andmedia-upload
).Part of #18838
How has this been tested?
Type check passes. No runtime changes except converting some
function
s to arrow functions so that we can type them asApiFetchMiddleware
instead of having to annotate the parameter types manually (see the changes to thenonce
middleware for an example of this).Types of changes
Non-breaking changes.
Checklist: