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

change req.params to an object instead of an array #1835

Merged
merged 1 commit into from Jan 3, 2014

Conversation

Projects
None yet
8 participants
@tj
Copy link
Member

commented Nov 28, 2013

this seems to trip people up a lot even though it behaves the same an object. technically a breaking change, though the impact would probably be extremely minimal - it might be best to wait until a 4.0 or we'll get lots of flack from trolls

@aheckmann

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

WHAT IS GOING ON???!?? hahaha

@defunctzombie

This comment has been minimized.

Copy link
Contributor

commented on c6c71ab Nov 28, 2013

Hm, I dunno. This changes the api and I think we documented req.params as an array so this would technically be a major bump?

@tj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2013

yeah probably best to wait :( sucks that major bumps without any new/cool/interesting features are sort of ill-received by most communities, wish that wasn't the case. I think that's a super broken model really ideally you'd iterate faster and break when you need to but people will be like WOW EXPRESS 4 IS OUT, oh wait it does nothing new

@defunctzombie

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

Welcome to the lands of semver!

@aheckmann

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

Yeah same for mongoose 4. there's really nothing stopping us from going all
browser on versioning though.

On Wednesday, November 27, 2013, TJ Holowaychuk wrote:

yeah probably best to wait :( sucks that major bumps without any
new/cool/interesting features are sort of ill-received by most communities,
wish that wasn't the case. I think that's a super broken model really
ideally you'd iterate faster and break when you need to but people will be
like WOW EXPRESS 4 IS OUT, oh wait it does nothing new


Reply to this email directly or view it on GitHubhttps://github.com//pull/1835#issuecomment-29438348
.

Aaron
@aaronheckmann https://twitter.com/#!/aaronheckmann
soundcloud.com/ajhecky
github.com/aheckmann

@rlidwka

This comment has been minimized.

Copy link
Member

commented Nov 28, 2013

Welcome to the lands of semver!

I thought nobody cares about semver, and all people bump minor version on breaking changes instead of major anyway.

@hallas

This comment has been minimized.

Copy link

commented Nov 28, 2013

just wait with this, no compelling reason to hurry it out the door as a minor

@Swatinem

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

there's really nothing stopping us from going all browser on versioning though.

True :-D

@tj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2013

haha ya im +1 on going browser, major bumps are more of an indication of failure than anything else, otherwise it would be minor-levels all the way through

@defunctzombie

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

You know what else is monotonically increasing ... dates

Anyhow. I think going full major with patch level is cool. I never really
bought I to semver 100%. More of a guide anyway. Hell even node core
doesn't follow real semver. Everyone does whatever they want.
On Nov 28, 2013 12:41 PM, "TJ Holowaychuk" notifications@github.com wrote:

haha ya im +1 on going browser, major bumps are more of an indication of
failure than anything else, otherwise it would be minor-levels all the way
through


Reply to this email directly or view it on GitHubhttps://github.com//pull/1835#issuecomment-29477881
.

@tj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2013

it should be <fail>.<feature>.<fix> IMO and all three should be independent counters so that you know when a feature and a bugfix were added in a release, and breaking change failures would still suck but if we did them more often at least it would be one or two like these small req.params res.locals things

@rlidwka

This comment has been minimized.

Copy link
Member

commented Nov 28, 2013

IMO and all three should be independent counters so that you know when a feature and a bugfix were added in a release

You mean, like 3.126.4233 ?

@Swatinem

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

More like 3.141.5926

@defunctzombie

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

If they are independent, what happens if you do a backport of a fix to an older major?

Lets say we are on 4.123.345 and we make a fix putting it at 4.123.346

And we want to back port this fix to 3.x but the last 3.x release we have is 3.120.340

Would it then become 3.120.341 ? or 3.120.346 ? skipping the things we didn’t backport?

On November 28, 2013 at 12:49:30 PM, TJ Holowaychuk (notifications@github.com) wrote:

it should be .. IMO and all three should be independent counters so that you know when a feature and a bugfix were added in a release, and breaking change failures would still suck but if we did them more often at least it would be one or two like these small req.params res.locals things


Reply to this email directly or view it on GitHub.

@tj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2013

probably just keep rolling the values up instead of having the value represent the # of the bug, i dont know, screw versioning hahah

@jonathanong

This comment has been minimized.

Copy link
Member

commented Nov 28, 2013

let's use autoprefixer's versioning. <let's not use this>.<something somewhat significant>.<the freaking date>.

@tj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2013

dates aren't really useful either as far as informing goes, might as well just be an incremental build number then. I think it's useful to see "we just released x.x.x" and know what has changed from that alone

@defunctzombie

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

I kinda like the idea of

<major shit changed, you probably don’t want to upgrade yet>.<minor shit changed… also can be breaking change, just not total rewrite of api>.<probably safe to update.. little changes but still RTFD don’t auto update>

For me, this goes along with my feelings on pinning dependencies. When I see a major bump, I typically think I need to investigate a serious change. Minor bumps to me require investigation and “may” have code impact, but are typically more targeted than “we decided to re-think our entire approach” and the last one also requires investigation but usually falls under, “little” to “no” code impact. The idea of a “patch” or “security” update does not exist because sometimes a security fix requires changes which are non trivial. Computers cannot read our minds yet!

All changes should be reviewed when using libraries because any change (no matter at what) dot level is a potential change in expected behavior. The dot levels to me just signify a bit of scope but don’t indicate that things won’t break.

Good changelogs and wiki pages really help with all of the above when updating. Updates should never “just happen automatically”.

So for this specific PR it would simply be a bump to 3.5 since we are changing behavior but not rethinking how express works. We document it, and make it very clear what changed and easy to find this changelog.

And we should also not hesitate to bump major version numbers when we feel appropriate but it is important to not just go changing everything all the time just cause. Express is written about and used in many many places now so we should be mindful of the “annoyance” of silly upgrades.

On November 28, 2013 at 3:23:43 PM, TJ Holowaychuk (notifications@github.com) wrote:

dates aren't really useful either as far as informing goes, might as well just be an incremental build number then


Reply to this email directly or view it on GitHub.

@aheckmann

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

People resist bumping major b/c it implies cost to upgrade and often an
assumption is made by users that the previous major version will soon be
unsupported. In that case, having a major version support policy is wise
(how long each major version will be supported) so we are free to iterate
as fast as we like but with "guarantees" for users of the software.

On Thu, Nov 28, 2013 at 12:36 PM, Roman Shtylman
notifications@github.comwrote:

I kinda like the idea of

<major shit changed, you probably don’t want to upgrade yet>.<minor shit
changed… also can be breaking change, just not total rewrite of
api>.<probably safe to update.. little changes but still RTFD don’t auto
update>

For me, this goes along with my feelings on pinning dependencies. When I
see a major bump, I typically think I need to investigate a serious change.
Minor bumps to me require investigation and “may” have code impact, but are
typically more targeted than “we decided to re-think our entire approach”
and the last one also requires investigation but usually falls under,
“little” to “no” code impact. The idea of a “patch” or “security” update
does not exist because sometimes a security fix requires changes which are
non trivial. Computers cannot read our minds yet!

All changes should be reviewed when using libraries because any change (no
matter at what) dot level is a potential change in expected behavior. The
dot levels to me just signify a bit of scope but don’t indicate that things
won’t break.

Good changelogs and wiki pages really help with all of the above when
updating. Updates should never “just happen automatically”.

So for this specific PR it would simply be a bump to 3.5 since we are
changing behavior but not rethinking how express works. We document it, and
make it very clear what changed and easy to find this changelog.

And we should also not hesitate to bump major version numbers when we feel
appropriate but it is important to not just go changing everything all the
time just cause. Express is written about and used in many many places now
so we should be mindful of the “annoyance” of silly upgrades.

On November 28, 2013 at 3:23:43 PM, TJ Holowaychuk (
notifications@github.com) wrote:

dates aren't really useful either as far as informing goes, might as well
just be an incremental build number then


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1835#issuecomment-29484259
.

Aaron
@aaronheckmann https://twitter.com/#!/aaronheckmann
soundcloud.com/ajhecky
github.com/aheckmann

@tj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2013

yea so much baggage goes along with major bumps, people expect new things, think old stuff is unsupported, feel pressured to upgrade, blerggghh

@tj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2013

plus it's one of those things that if we did do a 4.x soon, there's a lot of other breakage we'd want to get in while we can haha

@defunctzombie

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

I hate software development. I quit. I am going back to basket weaving.

On November 28, 2013 at 4:00:49 PM, TJ Holowaychuk (notifications@github.com) wrote:

yea so much baggage goes along with major bumps, people expect new things, think old stuff is unsupported, feel pressured to upgrade, blerggghh


Reply to this email directly or view it on GitHub.

@tj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2013

me too, basket weaving it is

@jonathanong

This comment has been minimized.

Copy link
Member

commented Nov 28, 2013

0.12 needs to hurry up!

@defunctzombie

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

I am + 1.0 on this being 3.5 and us breaking from the dumb semver pattern.

I am - 0.5 on this change since I don't see it being the problem it is described as but really have no strong feeling either way.

@jonathanong

This comment has been minimized.

Copy link
Member

commented Nov 28, 2013

for this particular issue, people who complain (the ones who care) would probably see it as a bug fix, anyways.

@defunctzombie

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

We should get better about updating the history file even before a release. Whenever some relevant API change or behavior change happens we should out it in that file. Stuff like "update connect" seems less relevant in that file and just adds to noise. This would be a perfect candidate for that file.

@tj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2013

it's relevant if you've been waiting for express to bump for some fix in connect

@guybrush

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2013

one cool thing with express is that it has a very good History.md in it, also i like things like "update connect"

if one really cares about changes github helps a lot 3.4.4...3.4.5

@jonathanong jonathanong referenced this pull request Nov 30, 2013

Closed

Express 4.0 #1838

8 of 11 tasks complete

jonathanong added a commit that referenced this pull request Jan 3, 2014

Merge pull request #1835 from visionmedia/change-req-params-to-object
change req.params to an object instead of an array

@jonathanong jonathanong merged commit 3f14b4d into master Jan 3, 2014

1 check passed

default The Travis CI build passed
Details

@defunctzombie defunctzombie deleted the change-req-params-to-object branch Feb 12, 2014

rlidwka pushed a commit to rlidwka/express that referenced this pull request Aug 6, 2014

Merge pull request expressjs#1835 from visionmedia/change-req-params-…
…to-object

change req.params to an object instead of an array
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.