Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Make MEAN less restrictive about Node and NPM #258

Merged
merged 2 commits into from
Dec 30, 2014
Merged

Make MEAN less restrictive about Node and NPM #258

merged 2 commits into from
Dec 30, 2014

Conversation

ilanbiala
Copy link
Member

Node and NPM shouldn't be just 0.10 and 1.4, because there are other versions of Node above 0.10. Also NPM is easily past 1.4, and people should be allowed to use 2.0 freely if they desire without having warnings or errors.

@lirantal
Copy link
Member

The only issue I have with ">= X.Y" rule is that it's quite possible that at one point in the future Node or NPM will update to some version that may break the way some node modules are installed, and when someone will do a fresh checkout, or will try to update existing node modules it might break.

The idea of pinning versions is good because you are essentially freezing to a known-to-work version regardless of the point of time that software will be installed.

So bottom-line how about replacing the >= and instead trying a = operator to match a version of NPM and Node which work well with MEANJS and all of its dependencies? then we can re-visit updating the versions as they get released and compatible with our framework.

@ilanbiala
Copy link
Member Author

I shouldn't have to work around tons of deps issues just so I can have npm 2 on my system but npm 1.4 for this project. That is what ci is for, so you can see if there are any bugs with different node/npm runtimes.

@ilanbiala
Copy link
Member Author

I just updated Travis CI to also check against 0.11, so you guys can see whether it will be compatible in the future without having to change any code. NPM is trickier, but I think that you should just let it be at least "x", and users will see whether it works or not. Eventually NPM 1.4 will be unsupported, so there is no point in preventing adventurous people from easily being able to try and hack on it.

@rschwabco
Copy link
Member

I think pinning versions is a good idea, and those should be closer to the latest stable release for that dependency as possible. I would definitely not allow automatic major version updates, but we could allow minor version updates with little to no issues. @amoshaviv @lirantal @ilanbiala wdyt?

@ilanbiala
Copy link
Member Author

I think that pinning it isn't a bad idea, but I wouldn't prevent people from using a different version for node or npm if it works. The other dependencies like Express should definitely be "~", but if it works there is no reason to stop people from trying out other versions of Node or npm if they want to have other features, especially since Travis can test against other versions of Node, it should be clear after any commit if there are issues.

@ilanbiala
Copy link
Member Author

@amoshaviv @roieki I think we should at least test against 0.10 and 0.11. Even if it means that we do 0.10.x for Node, it's good to know whether things will be working or not in future versions.

Don't forget that once people download the repository, there's nothing stopping them from changing the dependency to something else, so you really might as well make it a little less restrictive and just test against a few more versions.

@amoshaviv
Copy link
Contributor

Problem is 0.11.14 is the last known version, but should we invest time checking it on 0.11 instead of waiting for 0.12? Did you do an initial test for this?

@ilanbiala
Copy link
Member Author

Yeah my branch with 0.10 and 0.11 tests passes, but that was before some changes to other parts of the framework.

Note that just because the version changes, doesn't mean that Node changes. It means that the features are now stable enough to be considered for stable use. Odds are unstable builds, evens are stable builds. 0.11 is just the development branch and will become 0.12. 0.12 has been in the making for a while now, and waiting for 0.12 is not a good idea.

Testing against the last stable build, the current stable build, and the current development build is always a good idea. Because those are the most used versions. Ideally we should also be testing against 0.8 until a couple months after 0.12 has come out, because 0.8 is still technically supported.

@ilanbiala
Copy link
Member Author

@amoshaviv with all the Node forking going on, I think we should test against as much as possible at the beginning and then make it more restrictive as we run into errors that we cannot solve. Starting with the most restrictive doesn't make sense, because not everyone can run on the last x versions.

@ilanbiala
Copy link
Member Author

@amoshaviv @roieki @lirantal @NeverOddOrEven any thoughts on this? Is something like this ever going to be merged to test future versions of Node and npm?

@lirantal
Copy link
Member

but why is nodejs's travis set to node version 0.11 when 0.10.x is the current stable build? updating to npm v1.4.28 doesnt seem like a problem to me

@ilanbiala
Copy link
Member Author

@lirantal I added 0.11, which tests both 0.10.x and 0.11.x. I'm not sure how we can test with npm 1.x and npm 2.x, but I don't think anything has broken there that would affect MEAN.

@lirantal
Copy link
Member

then lgtm. unless anyone has anything else to add in a reasonable time, I'll merge it.

@NeverOddOrEven
Copy link
Contributor

The converse to your argument @ilanbiala "there's nothing stopping them from changing the dependency to something else" would be that the person pulling the repo could make it less restrictive if they so chose. I don't feel strongly one way or the other in this case.

I do agree with @roieki that allowing major upgrades could be very problematic. For now though, I don't see any major releases for node on the horizon, and if npm 2 works for you now @ilanbiala, then I don't see any issues there either.

Let's merge this for now, and if it becomes a problem later I will then support pinning the versions down.

LGTM

@ilanbiala
Copy link
Member Author

@NeverOddOrEven yep I agree with you on npm, but for Node I definitely think it's at least worth testing on 0.11 to be aware of the future.

@NeverOddOrEven
Copy link
Contributor

To clarify, by major I mean a 1-dot release lol. Kind of the white elephant in the community, or the butt of a seriously long winded joke...

lirantal added a commit that referenced this pull request Dec 30, 2014
Make MEAN less restrictive about Node and NPM
@lirantal lirantal merged commit 6a03450 into meanjs:master Dec 30, 2014
@lirantal
Copy link
Member

@NeverOddOrEven @ilanbiala I merged, so let's track the builds now and make sure that things aren't breaking.

@ilanbiala
Copy link
Member Author

:)

pdfowler pushed a commit to pdfowler/mean that referenced this pull request Jan 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants