search

AvianFlu / ncp

Asynchronous recursive file copying with Node.js.
MIT License
680 stars 103 forks source link

Fix the version check for modern node versions. #80

Closed nicks closed 2 years ago

nicks commented 9 years ago

Fixes https://github.com/AvianFlu/ncp/issues/79

bajtos commented 9 years ago

FWIW, this is a duplicate of #78. The approach presented here is more robust since it uses semver instead of regex. On the other hand, it adds an extra dependency to the project.

nicks commented 9 years ago

ya, i saw that. It seemed weird to assume that all node versions will be 1.x.x or 0.1x.x. What if there's an io.js version 2? or a nodejs version 0.20?

bajtos commented 9 years ago

Frankly, I don't care which pull request ends up being landed. The most important for me is to get a fixed version of ncp in npmjs ASAP.

/cc @AvianFlu

shakiba commented 9 years ago

+1 for this one!

chucksellick commented 9 years ago

Version sniffing seems a fragile way to check this kind of thing. How about just checking for the existing of setImmediate? (Not specifically related to ncp, but might it be a good idea for node/iojs to actually provide an api to test for feature existence in cases where significant functionality is introduced between versions? This would allow a robust mechanism to switch behaviour where back compat is required. And another strategy would be to branch ncp to maintain a pre-0.10 version and a post-0.10 version, and specify the compatibility in package.json accordingly. npm could even be able to locate the correct version for the installed engine.)

bajtos commented 9 years ago

to branch ncp to maintain a pre-0.10 version and a post-0.10 version, and specify the compatibility in package.json accordingly. npm could even be able to locate the correct version for the installed engine.

:+1: :x: :100:

Considering that v0.10 has been around for almost two years now, I believe we can afford to stop supporting v0.8 in new releases. My argument is that people running on Node v0.8 must be extremely conservative, most likely they won't want to make unnecessary changes to their running system, and thus they won't want to upgrade ncp to a newer version either.

We should consider releasing a new major version (2.0) after we drop support for v0.8.

chucksellick commented 9 years ago

I agree; and we already have "engines" to safeguard. (+ advantage it's easy to specify node.js and io.js versions independently). Anyone upgrading modules (esp. major version) carelessly is going to get bit sooner or later anyway...

capaj commented 9 years ago

@chucksellick :+1: probably would be good to get going with the solution you proposed with two forks. A lot of stuff is broken now due to this one lame regex.

bajtos commented 9 years ago

Considering the unresponsiveness of project maintainers, it may be worth switching to cpr instead.

Jonahss commented 9 years ago

Just a warning to others, cpr isn't a drop-in replacement for ncp. Copying single files works differently. Will this get merged?