Skip to content

Conversation

tanqhnguyen
Copy link

#2603

A simple change but it might be useful in some cases where we can write shorter code to reuse middleware in app.param

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use req.params as a bag-o-things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[key].concat(fns) will inadvertinately allow for the arguments to be arrays, because it'll flatten in certain cases, i.e.:

$ node -pe '[2].concat([1,2])'
[ 2, 1, 2 ]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is how apply works. I mean we need an array for apply to work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about concat; you need to use something else that won't flatten the fns array.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, nvm, I didn't look all the way, sorry!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. (Not related) Is my new test case alright?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not related) Is my new test case alright?

Yep

@tanqhnguyen tanqhnguyen force-pushed the multiple-fns-app-param branch from c501a05 to 78caf4d Compare May 14, 2015 23:20
@dougwilson dougwilson added the 4.x label May 14, 2015
@tanqhnguyen tanqhnguyen force-pushed the multiple-fns-app-param branch from 78caf4d to 6b01ff8 Compare May 14, 2015 23:35
@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 5f268a4 to 9848645 Compare July 31, 2015 20:58
@tanqhnguyen tanqhnguyen force-pushed the multiple-fns-app-param branch from 6b01ff8 to 840ed23 Compare January 18, 2016 19:57
@tanqhnguyen
Copy link
Author

@dougwilson Is this one still valid?

@jonchurch
Copy link
Member

From what I can tell, app.param is being removed from Express in v5, so unless there are still plans to land this in 4.x, this PR can likely be closed.

@dougwilson
Copy link
Contributor

dougwilson commented May 13, 2020

So app.param is not being removed in 5.0. In fact, it is being updated to include Promise support.

@jonchurch
Copy link
Member

Ahh thank you @dougwilson, I see now that this commit was reverted by this later commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants