search

sindresorhus / find-up

Find a file or directory by walking up parent directories
MIT License
582 stars 40 forks source link

Upgrade engines field to Node.js >=8.3 support #48

Closed LinusU closed 4 years ago

LinusU commented 4 years ago

This package have never actually supported Node.js 8.0.0 - 8.2.1 so this shouldn't be a breaking change, the de facto oldest supported Node.js version have always been 8.3.0.

sholladay commented 4 years ago

This has to do with our usage of async await, right?

Agreed that it's probably not a breaking change, in that case. Still might be polite to treat it as semver major. Who knows, someone might have engineStrict set to true in their npm config.

LinusU commented 4 years ago

This has to do with our usage of async await, right?

No, it's actually the usage of the spread operator:

https://github.com/sindresorhus/find-up/blob/6c32f0caed1684ef778053b6f79b13a772e22ba4/index.js#L29

Who knows, someone might have engineStrict set to true in their npm config.

Node.js 8.2.1 fails even trying to parse the file, so someone who is running an old version of Node.js and are using engineStrict cannot have had a working program before 🤔

sholladay commented 4 years ago

Someone could absolutely have engineStrict without hitting a parse error before. find-up is often nested deeply within a dependency tree. It's very plausible that somewhere along the way, find-up is imported dynamically based on a conditional, either directly or indirectly, or lazy loaded (perhaps with a try catch)..

It's an edge case, to be sure. But I thought it was worth mentioning because this module has 28 million weekly downloads and increasing. People use it in many different ways since it's such a low-level building block and one of its use cases is to find files that may or may not exist. It wouldn't surprise me if someone was getting fancy with how they import this module. I doubt many of those people are also using engineStrict, though.

LinusU commented 4 years ago

True 👍

I wonder how this would affect upstream bumping to this version then 🤔 Potentially they would have to make a major version of their library, bumping their version to 8.3 🤔

sholladay commented 4 years ago

The safe thing to do is to bump the major any time a dependency increases a version in engines, even if it's an indirect dependency, unless your own engines already excludes that version. It's not easy to know when that happens, though. There are a handful of tools that try to detect it, but most of them aren't very good.

coreyfarrell commented 4 years ago

See https://github.com/sindresorhus/meta/issues/8.

sindresorhus commented 4 years ago

Going to pass on this. I'll soon make this package target Node.js 10.