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

tj
Copy link
Member

@tj tj 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
Copy link
Contributor

WHAT IS GOING ON???!?? hahaha

@tj
Copy link
Member Author

tj 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
Copy link
Contributor

Welcome to the lands of semver!

@aheckmann
Copy link
Contributor

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
Copy link
Member

rlidwka 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
Copy link

hallas commented Nov 28, 2013

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

@Swatinem
Copy link
Contributor

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

True :-D

@tj
Copy link
Member Author

tj 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
Copy link
Contributor

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
Copy link
Member Author

tj 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
Copy link
Member

rlidwka 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
Copy link
Contributor

More like 3.141.5926

@defunctzombie
Copy link
Contributor

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
Copy link
Member Author

tj 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
Copy link
Member

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

@tj
Copy link
Member Author

tj 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

tj 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
Copy link
Member Author

tj 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
Copy link
Contributor

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
Copy link
Member Author

tj commented Nov 28, 2013

me too, basket weaving it is

@jonathanong
Copy link
Member

0.12 needs to hurry up!

@defunctzombie
Copy link
Contributor

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
Copy link
Member

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

@defunctzombie
Copy link
Contributor

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
Copy link
Member Author

tj commented Nov 28, 2013

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

@guybrush
Copy link
Contributor

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 mentioned this pull request Nov 30, 2013
11 tasks
jonathanong added a commit that referenced this pull request Jan 3, 2014
change req.params to an object instead of an array
@jonathanong jonathanong merged commit 3f14b4d into master Jan 3, 2014
@defunctzombie defunctzombie deleted the change-req-params-to-object branch February 12, 2014 16:22
rlidwka pushed a commit to rlidwka/express that referenced this pull request Aug 6, 2014
…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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants