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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: Changing arguments based on arity is unclear #2990

Closed
kevinburkeshyp opened this issue May 9, 2016 · 5 comments
Closed

api: Changing arguments based on arity is unclear #2990

kevinburkeshyp opened this issue May 9, 2016 · 5 comments

Comments

@kevinburkeshyp
Copy link

Calling app.use with a function that takes 4 arguments makes Express treat the first argument as a err parameter:

    app.use(function (err, req, res, next) {
      Logging.error(err, 'Response handler threw exception');
    });

Calling it with 3 arguments means the first argument is now a req and not an err:

    app.use(function (err, req, res, next) {
      Logging.error(err, 'Response handler threw exception');
    });

This is misleading - at least it was to me. I can imagine a scenario where someone deletes unused variables from the end of a router ("oh, next/res is unused here, we can delete it"), and accidentally changes the meaning of the other function parameters.

The 4-argument form is also undocumented in app.use.

@dougwilson
Copy link
Contributor

Hi @kevinburkeshyp, it can be confusing, we agree! There is a current discussion in #2896 regarding this issue. If you would like to read the existing discussion, proposals, etc. please feel free to do so and even add your own thoughts, proposals, comments on existing proposals, etc.

As for the documentation for the way is works currently, please feel free to create an issue regarding where in the documentation it is confusing, thoughts on what can be added to the documentation, pain points, etc. at https://github.com/expressjs/expressjs.com/issues

@dougwilson
Copy link
Contributor

I took a quick look over the documentation on the web site, and I think that they tried to make it clear, but perhaps you can provide a pull request for a suggestion of how to can be clearer. For example, I looked at the documentation and this is what I found:

  1. Went to the documentation on app.use (http://expressjs.com/en/4x/api.html#app.use).
  2. First sentence is "Mounts the specified middleware function or functions at the specified path" where it contains a link to what a middleware function is.
  3. I clicked on the link for what defines the function's make up and it lead me to http://expressjs.com/en/guide/using-middleware.html
  4. I clicked on "Error-handling middleware" to lean what makes an error handing middleware.
  5. There is a yellow box as the first thing in the section saying "Error-handling middleware always takes four arguments. You must provide four arguments to identify it as an error-handling middleware function. Even if you don’t need to use the next object, you must specify it to maintain the signature. Otherwise, the next object will be interpreted as regular middleware and will fail to handle errors."

I'm sure there was some reason you got tripped up in making those connections, and we would absolutely love to hear your feedback at https://github.com/expressjs/expressjs.com/issues on how to make it better & more straightforward. Of course, inlining that information into app.use directly is going to be tedious, since there are a whole list of APIs that accept "middleware functions", which is why we decided to document what makes a "middleware function" in a central page that all the other APIs reference.

@kevinburkeshyp
Copy link
Author

Sorry - I guess I'd expect to see at least one example or code sample that uses the 4 argument form. I clicked thru some of the links but not all of them. I missed the middleware link, and the links to app and router seemed unpromising.

@kevinburkeshyp
Copy link
Author

(Also sorry for not checking the GH issues before posting - thanks for the link to the other ticket.)

@dougwilson
Copy link
Contributor

@kevinburkeshyp that is some great feedback about the website! Can you post the feedback to https://github.com/expressjs/expressjs.com/issues ?

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

No branches or pull requests

2 participants