search

sindresorhus / tempy

Get a random temporary file or directory path
MIT License
423 stars 26 forks source link

Fix support for Node.js 8.0.0 - 8.2.1 #17

Closed LinusU closed 4 years ago

LinusU commented 4 years ago

blocked on https://github.com/xojs/xo/pull/408

sindresorhus commented 4 years ago

Sorry, not interested in this. Node.js 8.3.0 came out more than 2 years ago. It's expected that people upgrade patch/minor releases.

LinusU commented 4 years ago

Fair enough 👍 do you accept PR to upgrade engines to >=8.3?

sindresorhus commented 4 years ago

What problem are you trying to solve?

LinusU commented 4 years ago

I have a package that I've indicated support for node: >=8 on, and are thus testing on Node.js 8.0.0. I was a bit surprised when it failed since I had already checked all the engines field prior to bumping the packages.

To be fair, I could just update my package to have node: >=8.3, but I figured it would be a) nice for the next person who runs in to this to update it here as well, and b) nice for npm to print a warning if someone is running Node.js 8.0.0 and tries to install this package...

LinusU commented 4 years ago

closing in favour of #18

sindresorhus commented 4 years ago

I have a package that I've indicated support for node: >=8 on, and are thus testing on Node.js 8.0.0.

Sounds like this actually caught a problem in your testing setup. If you're testing on exactly 8.0.0 (which you definitely shouldn't), you're not actually testing on the reality of what users are using. Node.js 8 saw many V8 upgrades, bug fixes, and changes. Node.js 8.0.0 is a very different beast from Node.js 8.16.0.

npm to print a warning if someone is running Node.js 8.0.0 and tries to install this package...

From what I can remember, npm doesn't print a warning about it. Only Yarn does.

LinusU commented 4 years ago

If you're testing on exactly 8.0.0

I'm not actually, just did a test on that one as well just to see that it worked, since that's what I indicate in my package.json.

I don't really have any strong opinions on how this should be done. But it seems better to indicate what versions are actually supported, and then not break that later in a patch release. I think that the reality is that some people are stuck on certain minor versions (e.g. Lambda never upgraded from Node.js 8.10.0), and it seems wrong to break that compatibility in a patch release 🤔

e.g. would it be okay to release a new patch version that only works with 8.16.2? It feels like it shouldn't since realistically that could probably break for many people. Probably this specific instance have never happened before since not many people are running 8.0.0, still I don't see the harm in specifying the correct version in the engines field.

Personally, I would probably add 8.3.0 to my test matrix, just to gain some more confidence that I don't use something that's only available in a recent patch release, but that's just me ☺️