-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
Deprecate arity check in favor of explicit error handlers #59
base: 2.0
Are you sure you want to change the base?
Conversation
Nice! One thing I'm not sure about is the migration path for people with error handlers in the middle of a bunch of handlers. For example |
Yeah, that would be broken with this approach for sure. Is that something we could be alright with? The change they would have to make is to break those apart into multiple calls: app.use(loadSession)
app.error(sessionErrorHandler) |
Maybe. Of course, your example would now mean that session is going to initiate for all the routes instead of only the necessary ones, meaning this will likely start making all middleware out there build in route white-lists to get it working back like it used to, right? No need to hit the database to load sessions when the session is not necessary, right? |
You could also do those route chains with router instances: var r = new Router();
r.use(loadSession)
r.error(sessionErrorHandler)
app.use('/some-route', r) |
So the idea is that every route in the app is a new router instance? That would work for sure, I guess :) Certainly need to somehow understand the impact of removing a feature if it has no replacement. Certainly removing |
Not desirable for sure. Technically it would only be necessary for the uses where you want to mix error handler middleware in the middle. Maybe we could document and improve the var r = Route('/')
r.all(/* ... */)
r.error(/* ... */)
app.use(function (req, res, next) {
r.dispatch(req, res, next)
}) We could make it so you can just pass a
If you like the general direction here, I can write an update guide for sure. |
That could work, but it'll still work on all methods instead of just the |
I was just meaning that it is lighter weight than a whole router instance and can still, with only small modifications, suit the needs of grouping a set of middleware which includes error handling middleware together. Sorry my example was only meant to show how close it already is. |
Oh, and I forgot to mention that this PR added a bunch of other methods for handling errors on only one method: route.postError(function (err, req, res, next) {})
route.getError(function (err, req, res, next) {}) |
How do we feel about these changes? It will remove some use cases which are possible currently, but I think the cost of the arity check is worth losing these. I think that us taking a more hard line to improve the past api mistakes is a good thing. And in this case, the proposed api seems better in the long run. One other thing I noticed when re-going over this old PR. The example about making each route a new router instance is not what I intended. With this PR you could do: var r = new Router();
r.route('*')
.use(startSession)
.postError(customHandlingWhenPOSTing)
.error(sessionErrorHandler) Which I think better solves your concerns. I don't know why I didn't realize that was the better solution to the above questions. |
So what's the update on this issue? This not being merged is stalling the progress on #2896 in express. Will this be included in express 5.0? |
Obviously as the author of this I think this is a good direction to move, but I have not received any further feedback from @dougwilson or any of the other core maintainers. I would like to push some of these bigger breaking changes forward so that people know about them and have time to update before 5.0 drops, but I need either the ability to merge/publish these or some feedback on what is holding this up. |
I feel like the functionality you're removing is very useful, having seen many apps taking advantage of it. I think to move this forward maybe we need a solution that (1) doesn't remove so much functionality like inline error handlers and (2) passes the CI. |
The code in this PR provides an alternative (as shown above) with just a bit more code than previously. Is that not a satisfactory way? And I can fix the ci for sure, just didn't want to do it if we didn't agree on the actual feature. |
I guess it does remove the version where you package them up in an array. So it is not exactly the same, but again, I argue that this loss is VERY worth it for the gain of not having the arity check. |
Well, at the very least, we can't just remove something and not explain to folks how to migrate, or no one will know how to upgrade. I don't think it should be removed, but maybe if you can write up a migration guide on what folks are supposed to do instead that may help convince me otherwise. Here is what I see a lot of, to give you an example to work off-of: app.put('/foo/:id', authnMiddleware, authzMiddleware, parseMiddleware, loadFooMiddleware, putFooHandler)
// ... where the above is generally the following
authnMiddleware = [loadSessionMiddleware, sessionLoadErrorHandler]
authzMiddleware = [validatePermissions, permissionErroHandler]
parseMiddleware = [parsePayload, payloadErrorHandler]
loadFooMiddleware = [loadObjectMiddleware, loadErrorHandler] |
Also, to be clear I'm not arguing against the removing the the arity check -- I'm arguing against removing the ability to have these inline, mixed error handlers. |
Do you have any ideas on how to keep this feature but also remove the arity check? That is the problem I am having, I cannot think of a clean way to have both. |
We could introduce a new api which wraps error handlers, something like:
Or we could add a property:
What other types of solutions are there? |
I had one other idea for this last night. What if we expose a new abstraction, a
This avoids the loss in features but does introduce a new abstraction (which has learning and support downsides). I am really trying to think of the best solution, because I agree we don't want to loose functionality, but this arity check really needs to go lol. What do you think of this idea? |
Yea, I'm following. That does make sense: basically move away from a basic array with functions in it to an object that understands the difference. Not unlike a Promise object with .then and .catch actually. |
Exactly, that is what I was relating this idea to as well. Does this sound "promising" (pun intended) enough to build a PR around? IMO it is better than the other proposals I have above despite the added complexity of a new abstraction. |
Ok, I was thinking more about what this could look like, and this is what I came up with: var flatten = require('array-flatten')
var slice = Array.prototype.slice
class Stack extends Array {
constructor (handlers, errorHandlers) {
super()
if (handlers && handlers.length) {
this.use(...handlers)
}
if (errorHandlers && errorHandlers.length) {
this.error(...errorHandlers)
}
}
use () {
this.push(...flatten(slice.call(arguments)))
}
error () {
this.push(...flatten(slice.call(arguments)).map((fnc) => {
return ErrorHandler(fnc);
}))
}
}
function ErrorHandler (fnc) {
Object.setPrototypeOf(fnc, ErrorHandler.prototype);
return fnc;
}
var s = new Stack([() => {}], [function errH () {}]);
s.use(function bar () {});
s.error(function err () {});
s.forEach((f) => {
if (f instanceof ErrorHandler) {
console.log('err', f);
} else {
console.log('use', f);
}
}) What do you think? The stack inherits from array, so all the other code could remain the same, and for error handlers all we need is to pass them to the |
@wesleytodd that's nice, I was actually thinking of something similar just the other day, but using Symbols instead of the prototype. const kErrorHandler = Symbol('error-handler')
function ErrorHandler (fnc) {
fnc[kErrorHandler] = true
return fnc
} I guess that both would allow us to be backward compatible? That would be a really big plus in my opinion since we can then add this to Express.js 4.x. We just need to change the arity check to also look for the symbol/prototype: - if (handler.length === 4) {
+ if (handler.length === 4 || handler[kErrorHandler]) { One con with the One con with I'm happy with either one 👍 |
It would be in what we plan to support in |
I haven't read the above yet, but just wanted to chime in on the 5.x thing: there is pretty much no reason it would support anything below Node.js 4 at this point 🤣 |
Good idea! I think maybe that was because I pushed the change to remove the references to |
I was able to cancel it. I had to switch my phone from mobile to desktop and then the cancel actually canceled it. |
I have added an example for the conversion. Do you disagree on my thoughts about why it should be a separate module instead of bundled in here? |
Sorry I didn't realize you had the reason listed in here. I left to go shopping now, but i will be back on later tonight to go back up and find and read it and let you know. I assume I didn't already respond to it? |
Wouldn't the same apply to the other classes in this module? Since it would just be an export any reason it couldn't be used by requiring this module? My thought is that we should either split out everything or not. Having this single one be split out feels weird, but maybe if you can show an example of how it would be used specifically that could help make sense? |
My biggest concern to it being split is that we're going to have another path-to-regexp situation, where a core part of how this module works suddenly starts changing completely independently and it gets way out to sync. |
(That's not my only concern, at least jist one i typed really wuick while walking lol) |
Looking at the readme of mwstack it doesn't seem like you can do anything other than ultimately pass it to express. There doesn't seem to be any kind of interface to make use of the stack after building it. I assume this module would then use undocumented interfaces to make use of the object from that module. So that's partly why I'm not sure how anyone else would be able to make use of it outside of this module, as the readme of mwstack makes it read like that is the case. |
It might, and I see no issue with that where it makes sense.
There are pros & cons here. While I agree that
Another router could also use it, which is one of the first things I was planning on doing with it. The hope would be that my experimentation would help direct the future of this module.
It would be the same way we use it, just allowing for other implementations to directly share our implementation. I was going to mess with integrating it to my experimental router, so if you want I can prioritize that work, but it probably wont show much. |
Yes, there are definately pros and cons. But my opinion, based just on the mwstack repot itself, is that it is very tightly-coupled with this module. There is no reason it cannot just land here first and then be extracted after the fact once there is a need to share it without the baggage here. Right now that module has no documentation on how it would be possible to use without this module, so to me, seems like it hasn't even graduated to a point where it should be a separate module vs just an included class of this module. |
It is coupled to the API, but not the implementation details. My usage of it outside of router would be because I am going to do some things similar in API but different in implementation details. I would just have to use my separate version of it, which seems pointless to me 🤷♂️. |
What I mean, though, is that we want to treat separate modules as separate, and utilize the public APIs of each one, so the semver makes sense and we can manage them. This PR seems to be using undocumented APIs from the mwstack module currently. How can we manage something like that over time? If it's just a class within this module then it doesn't matter, but being a separate module we need to get everything in order if that is what we want to do, and that is something at the top of my mind for what is missing here. |
Which ones? If I missed one then it was an accident. |
Is it that I only say |
var fn = callbacks[i] | ||
Router.prototype.use = function use() { | ||
var args = processUseArgs(arguments) | ||
this.stack = this.stack.concat(createLayers(args[0], args[1], { |
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 .concat
seems to be undocumented in the mwstack
repo.
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.
Ok, yeah so that is just because it extends array. Would it satisfy if I added a section at the top of the api docs which linked to the array documentation and said "Stack and FlatStack extend from array"?
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 get that it extends an array, but there is no documentation for what that array actually contains. Is it an array of strings, numbers, objects of some kind, functions... ? I cannot tell if the result of createLayers
here results in something that should be in that array.
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.
It is an array of functions, but I guess I can see how this line abstracts that in such a way where it is hard to know. Let me see if there is a way I can make this more clear in the documentation. createLayers
returns the result of callbacks.map
, so it returns an array of middleware functions which are just passed to Array.prototype.concat
.
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.
So if .concat
should be given an array of function, then this API is used wrong here. The createLayers
function is returning an array of Layer
objects, not an array of functions.
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.
Yeah, Now that I read this again I think it is better to keep them separate. So I changed router.stack
back to a plain array. I will push once we have any other changes worked out (so I don't break any reviews in progress).
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 haven't dug into the PR deeply to comment on what makes sense or not; currently my comments are just about if we're splitting things apart into different modules, then we're careful to use the external module as is documented/support by said module so we don't end up breaking ourselves on accident with mysterious API usage is all.
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.
Perfectly understandable. I will make sure that mwstack
has a more clear documentation on the fact that it extends Array
. In this case, now that I am removing it's usage as router.stack
we are only importing it to expose it and use the one documented method mwStack.isError
. Seems like that solves the concern?
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.
Perhaps. I'm not sure without seeing it to honestly say for sure 😆 . To be clear: my concern is just that we're using a module as that module documents being used. The isError
seems to say the following from the mwstack
docs:
Returns true when the middleware was added to a stack as an error middleware.
It seems to imply, at least to me, that it is to be used on a func
that was added to the Stack
of this module, is that right? And it will tell you yes/no if it was added to Stack
as an error middleware or not?
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 it is used on a function which was never added to a stack as an error it will always return false
(so just any random mw function). But yes, it is intended to be passed any middleware function and will return true
/false
accordingly.
I will work to comment on them inline in the code, since I will need to review through all the code to note which ones they are. |
if ((!opts.error && !mwStack.isError(fn)) && fn.length === 4) { | ||
// not a standard error handler | ||
deprecate('registering error handlers with .use is deprecated, use .error or Stack.error instead') | ||
opts.error = true |
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 think this is an issue here, as it is modifying opts
but that is an object used for all callbacks
, yet the modification seems to be only for the given callback
being mapped over at the time.
It would seem to reason that if callbacks
was an array of "middleware", "error handler", "handler" then this would end up marking that last "hander" as an error handler incorrectly?
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.
Good catch! I will write a test which covers this and then make sure that the opts
are only modified locally.
Also, it is quite buried in this thread, but the goal of exposing the |
Yep, understood 👍 I recall that aspect of these changes and have no issue with it, being an edge-use-case. |
@dougwilson do you consider this |
Sorry, please remind me which issue in particular, as there were a couple conversations going on above in parallel. I know you said you were going to push up some change around isError or something so I was mostly just waiting to look when new changes appear, but are you asking to no longer make those changes? |
Sorry, I should have written more to be more clear. Yes there were three points:
I think I can resolve numbers 2 & 3 with the conversation we had above. If that is resolved, does it also resolve number 1 for you? I clearly think this belongs in a separate package, but if you disagree strongly enough to block this I would rather pull it in and get the ball rolling forward on fixing this than be pedantic on where the code lives. |
ping @dougwilson, I would love to land this because I think it is one of the more impactful feature changes, and if we dont land it in 5 we have to deprecate in 6 and remove in 7. Can you take a look so I can either pull the |
It sounds like it would, but only after seeing the changes could I truly say. I've been waiting to see what they look like...
It is not being "pedantic on where the code lives"; if something is split into it's own module it need to be treated well. It is extremely straight forward to pull things out at a later time and so it doesn't make sense to start with a weird module where we are immediately using undocumented things from it. |
I didn't realize you were waiting on that. I will for sure make the updates then. Including moving the code into this repo. I was saying that I did not want to be pedantic, so I am fine moving it into here if that helps this move forward. |
Gotcha. If you're ok with that, it seems like it would be pretty straight forward, and if it would help, I could add a commit that moves it (separate commit so you could review / remove if incorrect) if that would help speed things along :) If your next window is this weekend, I could get to it prior to then. |
Yep! I will do that as soon as I can carve out the time (hopefully tonight)! |
This is a first go at the idea discussed in expressjs/express#2896
It deprecates the
router.use
androute.<method>
use cases with a 4 argument handler function in favor of explicitly havingrouter.error
androute.<method>Error
.Thoughts?