-
-
Notifications
You must be signed in to change notification settings - Fork 511
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: add "listHandlers" method to server and worker #1369
Conversation
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 50f6b9b:
|
Hey, @AlexeiDarmin. Thank you for opening this on such a short notice! I absolutely agree with the summary proposal and |
src/node/createSetupServer.ts
Outdated
@@ -137,6 +137,10 @@ export function createSetupServer( | |||
) | |||
}, | |||
|
|||
listHandlers() { | |||
return currentHandlers |
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.
We need to be cautious exposing internals, and currentHandlers
is internals. I fear that developers will be tempted to meddle with this array directly. That should never happen. They should use explicit APIs like use
or resetHandlers
to affect the list of handlers, and listHandlers()
must be purely readonly.
Maybe let's make this array readonly before exposing it?
// First, guard the array on the type level.
listHandlers(): ReadonlyArray<RequestHandler> {
// Second, prevent mutations on runtime.
const immutableHandlers = [...currentHandlers]
Object.freeze(immutableHandlers)
return immutableHandlers
}
src/node/glossary.ts
Outdated
* Returns lists of all active request handlers. | ||
* @see {@link https://mswjs.io/docs/api/setup-server/list-handlers `server.list-handlers()`} | ||
*/ | ||
listHandlers(): RequestHandler<RequestHandlerDefaultInfo, MockedRequest<DefaultBodyType>, any, MockedRequest<DefaultBodyType>>[] |
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.
If we go to the readonly list route (which I think we should), let's adjust this return type as well.
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 absolutely love the changes! The only suggestion I have is to make sure that we expose a readonly array from listHandlers()
so that people are not tempted to mutate it.
Since we're not deprecating |
I've covered some of the previous suggestions, also added |
c8667f0
to
be9a076
Compare
@kettanaito thanks for applying the changes! I appreciate your feedback and was going to incorporate when I had a minute. |
Released: v0.46.0 🎉This has been released in v0.46.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
@kettanaito is something like this what you had in mind?
Change overview
This PR adds a
listHandlers()
method that returns a list of currently active handlers as suggested in #474 (comment).Additional change proposal
Now that devs would have access to individual handlers, handlers would benefit from having a
.toString()
or.stringSummary()
to easily see a string summary of the handler. The logging logic withinprintHandlers
could be moved tohandler.toString()
, thenprintHandlers
would simply be: