-
Notifications
You must be signed in to change notification settings - Fork 167
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
add Middleware support via server.middleware = [/*...*/]
#1089
Conversation
server.withMiddleware
server.withMiddleware
Just realized an even simpler API was staring me right in the face, similar to the API to set a namespace: routes() {
this.middleware = [
random500(),
addABunchOfResponseHeaders(),
// more middleware can be added here...
]
// Subsequent routes will have that middleware
this.get('/users', (schema, req) => {
return new Response(204, {}, null);
});
this.middleware = [
// differentMiddleware
]
this.get('/frogs', (schema, req) => {
return new Response(204, {}, null);
});
} I've update this PR to have this API. It still has |
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.
Hi @osi-jehrlich, thanks for the contribution! I have to admit that after the original maintainer of this project left, we're just a skeleton crew keeping the lights on, so I'm a bit reluctant to add new features. That said, this does seem like it could be pretty useful, and you've added both tests and types so I feel a bit more comfortable with it. I'd argue against adding two different APIs that accomplish the same thing, and your new API feels a bit more similar to how other parts of mirage work, so I'd suggest removing withMiddleware
.
I'd also like to hear what @cah-brian-gantzler thinks of this approach.
lib/route-handler.js
Outdated
() => { | ||
return this.handler.handle(request); |
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.
With this approach, if middleware wants to change the request, it needs to mutate the request
parameter directly before calling next()
, right? What would you think about also allowing middleware to call next(request)
, to be able to pass a cloned or even completely new request object into the subsequent handler? I think something like this could work, while still allowing next()
to be called without arguments, right?
() => { | |
return this.handler.handle(request); | |
(req = request) => { | |
return this.handler.handle(req); |
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 like the idea quite a bit; it'll need a couple more lines of code and type definitions, too - I'll get on that. I haven't made new Requests
from old Requests
, but as long as all the data is available to copy over it sounds sound.
Removing withMiddleware
seems like a good idea, too. I think it's easy enough for library consumers to write a standalone version of that if they need to once the simpler API is available. We can always add it at some point if there's demand.
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.
Updated :-)
…quest arg to next(req?), update types to match
server.withMiddleware
server.middleware = [/*...*/]
Is there anything else I can do to help? I haven't yet updated the documentation (api docs, guides, etc.), and I'm happy to do exactly that if we think this PR is on its way in. |
Thanks, I'd like to hear what @bgantzler thinks of this approach, but he's been busy lately. |
Given what I see Im fine with it. Future changes could be that the default handler function is supplied as the last element. While I have already extracted the default handlers so that we could supply a Pretender or the MSW variation. With this change Im wondering if there isnt a way to further extract that to make the Pretender and MWS version BE just another middleware (I realize they are since the handler is in the last spot, but I bet theres room for improvement and simplification of the abstract handler). Just a thought, could be wrong. Would have to explore to see if that could be done or not. I am fine with this PR. I understand it, and its a pretty simple change considering what its doing. |
FYI, I've created a PR with documentation to the repo for the main website. If you get a chance to take a look I'd appreciate any and all feedback. Is there anything else I can do (or need to do) to help move this PR toward merge/release? |
Hi, how to call async calls in middlewares? In my case i want to check jwt using await jose.jwtVerify(getJwt(request), jwtSecret) which seems not to work? |
Perhaps we can move this discussion to the miragejs/discuss repo? [Update]: I can't get async middleware to NOT work as long as it's aware that downstream middleware might return a promise. Perhaps the Typescript type definition needs to be updated to specifically state a |
Thank you @osi-jehrlich This worked for me! |
[![Mend Renovate logo banner](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [miragejs](https://togithub.com/miragejs/miragejs) | [`^0.1.47` -> `^0.1.48`](https://renovatebot.com/diffs/npm/miragejs/0.1.47/0.1.48) | [![age](https://developer.mend.io/api/mc/badges/age/npm/miragejs/0.1.48?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/miragejs/0.1.48?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/miragejs/0.1.47/0.1.48?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/miragejs/0.1.47/0.1.48?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>miragejs/miragejs (miragejs)</summary> ### [`v0.1.48`](https://togithub.com/miragejs/miragejs/releases/tag/v0.1.48) [Compare Source](https://togithub.com/miragejs/miragejs/compare/v0.1.47...v0.1.48) #### What's Changed 🚀 **Enhancements** - Add Middleware support via `server.middleware = [/*...*/]` by [@​osi-jehrlich](https://togithub.com/osi-jehrlich) in [miragejs/miragejs#1089 - Allow for keyForId and valueForId on the serializer by [@​cah-brian-gantzler](https://togithub.com/cah-brian-gantzler) in [miragejs/miragejs#1086 🐛 **Bugfixes** - Don't require testConfig for timing to respect environment. by [@​rmjohnson-olo](https://togithub.com/rmjohnson-olo) in [miragejs/miragejs#1080 - Update typing for `Request.queryParams` by [@​jasikpark](https://togithub.com/jasikpark) in [miragejs/miragejs#1027 🏠 **Internal** - Simplify lodash dependencies by [@​mansona](https://togithub.com/mansona) in [miragejs/miragejs#1091 #### New Contributors - [@​jasikpark](https://togithub.com/jasikpark) made their first contribution in [miragejs/miragejs#1027 - [@​rmjohnson-olo](https://togithub.com/rmjohnson-olo) made their first contribution in [miragejs/miragejs#1080 - [@​osi-jehrlich](https://togithub.com/osi-jehrlich) made their first contribution in [miragejs/miragejs#1089 **Full Changelog**: miragejs/miragejs@v0.1.47...v0.1.48 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 5am every weekday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/magicbell-io/magicbell-js). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy41OS44IiwidXBkYXRlZEluVmVyIjoiMzcuNTkuOCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Relating to Add Middleware support to Mirage #41 .
Middleware
This PR adds middleware to miragejs via a new
server.midleware = [...]
Example middleware which randomly returns a 500 response:
Routes which use the middleware defined above:
(Edited to match latest update)
Not included
There's a bit of funkyness when middleware wants to modify the response from a route handler because the route handler might return a
Response
object or it might return something else altogether. I imagine this could be addressed separately.async
middleware - again, could be added after (if this is the approach this repo heads in).