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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Experimental and ugly middleware support #267

Closed
wants to merge 2 commits into from

Conversation

asantos00
Copy link
Collaborator

@asantos00 asantos00 commented Jan 7, 2020

Motivation

Middlewares are a very flexible way of composing behaviour on web servers. Currently, mirage lacks support for it and it would be especially cool for plugins and shared behaviour to add it.

Description

This PR adds middleware support to miragejs. API is still experimental (and quite ugly). It doesn't even support "all the middleware features" that stuff like express offers. But here it goes:

new Server({
  middlewares: {
    pre: [/* middlewares that are going to be applied before the handler */],
    post: [/* middlewares that are going to be applied after the handler */]
  },
  routes() {
   // routes
  }
})

I agree that this is far from the perfect API, it should be something like the app.use from express and I'll try to go in that direction.

Regarding the middleware code itself, the API is the following (I also don't like that the API changes - the return value - for pre and post middlewares)

// pre middleware - adds `25` to every `request.params.id` before the `userHandler` is called.
const requestMiddleware = (request, response) => {
  request.params.id = parseInt(request.params.id, 10) + 25;
  return request;
}


// post middleware - adds `processedBy` field to every response
const responseMiddleware = (request, response) => {
  response.processedBy = 'miragejs';
  return response;
}

Questions/Problems

Currently, it's being hard to have a coherent API between post and pre mw APIs due to the handler's signature. It would be cool to migrate to something more similar to expressjs (req, res, next). And that would also make middlewares API simpler.

At the moment the fact that we also don't have methods like res.send, as there is on express, it is not possible to have a pre middleware directly answering the request. The best is can do for now is to enrich/mutate it.

Notes

This PR is obviously a draft, the APIs still have a lot to improve (as said before) but I'd like to have you guys opinion on what makes sense and what not, did this draft so we have something to discuss on top of.

I get that this might not even be possible/worth doing right now, due to the problems listed before and the priorities. Just had this idea in mind and wanted to put it out, who knows if something interesting comes out of this.

Now that I think of this, this would probably make more sense as an event emitter/subscriber than as a middleware 馃

Would love to hear your opinion

@asantos00 asantos00 changed the title WIP: Experimental middleware support WIP: Experimental and ugly middleware support Jan 7, 2020
@samselikoff
Copy link
Contributor

Thanks for opening this!

This feels more like a new feature that needs some discussion, so everyone can see it + hash out the API and throw in their thoughts.

I'm trying to keep miragejs's PRs actionable, so what do you say we open up a new issue over in https://github.com/miragejs/discuss?

@asantos00
Copy link
Collaborator Author

It definitely needs discussion! I'll open the issue there 馃槈

@samselikoff
Copy link
Contributor

Opened an issue: miragejs/discuss#41

I'm going to close this for now but we have a reference to it over there so we won't lose any of your work!

@samselikoff samselikoff closed this Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants