Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Dependency move-file only supports node >= 10.17 #37

Closed
BigsonLvrocha opened this issue May 8, 2020 · 8 comments
Closed

[BUG] Dependency move-file only supports node >= 10.17 #37

BigsonLvrocha opened this issue May 8, 2020 · 8 comments

Comments

@BigsonLvrocha
Copy link

What / Why

I can't upgrade a package depending of this package since I was using the same version of node (1.16.1) that the server uses, but it just throws an error that is hard to trace because incompatibility is in a subdependency.

When

When I run yarn install in server

Where

Where node is 10.16, supposedly compatible with this package

How

Current Behavior

yarn throws error that move-file is not compatible

Steps to Reproduce

just run

nvm use 10.16
yarn install

Expected Behavior

If move-file is really needed, then update node engine constraint to >= 10.17

References

Related to raineorshine/npm-check-updates#651
Related to npm/pacote#41
Broken by sindresorhus/move-file#8

@alexander-akait
Copy link

/cc @isaacs

@alexander-akait
Copy link

/cc @AnthonyWC @assapir @darcyclarke @ethomson @evilpacket @gordey4doronin @hilli @isaacs @lumaxis @owenniblock @radiantspace @ruyadorno

Somebody can look at this? It is very important.

When did we start using cacache in webpack we use 15.0.0 version and guaranteed work on 10.13 node, but in 15.0.1 package migrates from move-file to move-concurrently 92b1251 without major release, but it was breaking change becaise package requires 10.17 node (https://github.com/sindresorhus/move-file/blob/master/package.json#L14). This created problems for many developers (locally and on CI). Yarn doesn't work without additional flags.

I ask you to pay attention to this problem, thanks

@isaacs
Copy link
Contributor

isaacs commented Jun 2, 2020

Yeah, the intent was to support all 10.x versions, so that's definitely an oversight on my part, sorry. I'll take a look this afternoon.

@isaacs
Copy link
Contributor

isaacs commented Jun 2, 2020

So, the good news is, since cacache doesn't ever use move-file in such a way as to require making directories, so it doesn't hit the failure case. But still, it would be pretty easy to have a move-file that didn't rely on fs.promises and recursive fs.mkdir by just adding mkdirp as a dep.

@isaacs isaacs closed this as completed in bf88af0 Jun 3, 2020
@XhmikosR
Copy link
Contributor

Sorry for commenting on an old issue, but it's probably worth dropping mkdirp now and perhaps switching to fs/promises instead of util.promisify.

@wraithgar
Copy link
Member

The last time we looked into fs.promises it was significantly slower than util.promisify. This is usually not a big deal for apps that only do a few fs calls, but the cli is very fs-heavy and thus those slowdowns were unacceptable.

@wraithgar
Copy link
Member

Our current preferred way to use fs is through https://github.com/npm/fs which exports promisified versions of all methods by default, and of course has the polyfills we need for our node support matrix.

@XhmikosR
Copy link
Contributor

Thanks for the explanation! It might be worth switching to that then for all fs methods instead of util.promisify and mkdirp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants