builds with node 6.0.0 #402
Conversation
For the just-released node.js 6.0.0 there was also an update to nan, 2.3.2. This PR loosens the dependency on nan so that a 6-compatible version can be pulled in.
|
👍 |
|
All of the tests pass for me after this change. Node v6.0.0, OSX 10.11.4. |
| @@ -23,7 +23,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "bindings": "1.2.1", | |||
| "nan": "2.0.7" | |||
| "nan": "^2.0.7" | |||
There was a problem hiding this comment.
This is not a good way to specify dependencies. Please use exact versions to avoid breaking changes being introduced.
There was a problem hiding this comment.
Agree, but this is kind of an emergency with the Node6 release. This could go to "2.3.2" if that makes everyone feel better. If this is holding up cutting a release, I can send in a PR. I've got a bunch of code that is currently broken awaiting a release, so I'm motivated to help. :)
There was a problem hiding this comment.
I have a different perspective. Suppose the nan folks find and fix a bug, and release 2.3.3. By locking to 2.3.2 that means that you'd have to modify this file again (and cut another release) just to pickup that bugfix. I guess it depends on how well nan follows semver. Since it's part of core node.js now I have good faith that they'll follow semver fairly well.
Hmm... I guess it's a preference between being conservative versus being more trusting, so I can't speak against locking the version. Locking the version requires more attention and effort (to make these small changes to package.json) so my preference is to be more trusting of nan. After all, the plan is to release a new major version of node.js twice a year (which likely involves a new version of nan as well).
There was a problem hiding this comment.
Emergency or not I agree with @defunctzombie. For future compatibility this definitely should specify a static version before it can be released. I'll create a PR to fix this.
There was a problem hiding this comment.
@drewfish I have no faith in anyone. Mistakes happen. It is must more reliable to select a good working version and avoid issues where someone installed version X of module and I try to debug an issue but get version Y (patch, minor, or otherwise). This lets us specify the dependencies we have tested with and expect.
Additionally, I think it is a feature that we have to be explicit about dependency upgrades and cut a new release. This makes it clear when changes happened and downstream consumers are able to properly migrate or roll back without issues.
@hildjj There is no emergency here. I'm sure your code works on v5 and all this does is hold up a migration to v6 which is not an emergency given that it just came out days ago and that you can easily depend on a fork of this module if you need to for integration testing. Emergency is if we shipped a broken package under a patch release or something else critical breaking installs on versions of node that have been out for more than a day.
There was a problem hiding this comment.
I agree that it would not normally be an emergency, except that homebrew pushed Node6.
There was a problem hiding this comment.
@defunctzombie Finally found the thread where we discussed this.
Note to all: libxmljs is now using a tilde (~) before the NAN version in package.json.
Yes, @defunctzombie, this directly goes against what we both agreed, but a tilde will allow newer patch versions which I believe is okay since NAN does follow semver.
I think that allowing patch versions of NAN is important because of the way NAN header files interact with the differing versions of node-gyp that people have installed.
If a user has the latest node-gyp, and that introduces an issue with the NAN header files, NAN will release a patch and our lib will be fixed automatically.
Ideally, it will save us headache in the long run. Especially since patches usually get out faster for NAN than they do for libxmljs.
|
Try npm install nan first. I had the same issue and below command solved it for me. npm install nan@latest nan@2.7.0 is actually installed |
For the just-released node.js 6.0.0 there was also an update to nan, 2.3.2. This PR loosens the dependency on nan so that a 6-compatible version can be pulled in.