Blocking Node version check

[olivierlambert]

Hi there,

That's maybe a bug :)

Our project (Xen Orchestra) have a dep (chartist) we use in Browserify. But yarn stop at:

[2/4] Fetching packages...
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=5.5.0".
error Found incompatible module

This should be a warning, not an error, because we don't use this dep in Node.

Running this on Node v4.4.7 and Debian testing.

[ChristopherBiscardi]

I'll add that it seems that yarn --ignore-engines doesn't seem to help with this.

[sebmck]

@olivierlambert We're doing the right thing here. chartist has the required node version set to >=5.5.0 but you're running version 4.

[sebmck]

@ChristopherBiscardi Seems like a separate bug, can you open a new issue? Thank you!

[jkrems]

@kittens It's the right thing in theory. In practice it's the reason why pre-release versions of node can't install packages with engine-strict because 7.0.0-rc.1 is not in the range >=5. I would vote for making this a warning even though it might sound wrong.

[jlongster]

I just hit this as well. Apparently the engines field is "advisory" according to the docs: https://docs.npmjs.com/files/package.json#engines

I think you are right to error, but it's going to cause some conflicts with the current ecosystem where packages are too strict but nobody cared because it didn't error before.

I can also repro that --ignore-engines does not fix it. I can file a separate bug.

[olivierlambert]

@kittens it's not "the right thing" if you use this dep without Node (eg with Browserify). In this case, Node version doesn't matter because the dep won't be used in Node at all.

But display a warning is a good idea anyway.

edit: if there is a way to "tell" that it won't be used in Node that I never heard of (which is very likely), I'll be happy to do so.

[jlongster]

I just filed #638.

[jkrems]

This affects multiple of our (Groupon's) internal apps. Including things like "years ago someone set the node engine to 0.10 || 0.11 when they dropped support for node 0.8". But even if every package would have a perfect engine field, the error behavior is wrong for node prerelease versions and packages that aren't used in node. And then there's packages that use execSync (random example) in one code path but are completely fine to use w/o it in 99% of the cases.

I really don't want to have to patch yarn whenever I try out a beta version of node. And the only alternative is to add a boilerplate --ignore-engines to every single project. :/

[erikdonohoo]

Would love to at least have --ignore-engines flag work

[samccone]

as soon as a release is cut with
#647

Things will work with --ignore-engines.

--ignore-engines being opt in was very much intentional, the fact that these fields were only advisory actually removed all of their value forcing package like this to be invented https://github.com/samccone/engine-deps

Having packages respect this field is super super valuable.

[erikdonohoo]

Thanks @samccone. I don't disagree that package authors need to use that field for what it was intended. Looking forward to next release.

[jkrems]

@samccone Again, that's all great in theory. But the practice is things like this:

> yarn add angular2-cookie
yarn add v0.15.1
info No lockfile found.
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=5.0.0".
error Found incompatible module

There's nothing in that package that actually needs node 5+. If I understand it correctly they're using "node>=5" to express "we're super excited about npm 3!!!!". A project using angular 2 w/ the latest node LTS will currently not work with yarn. It works fine without it.

I agree that having a good, reliable way to tell a user "this package will definitely not work at all unless you use node X or higher" would be great. But because of the way engine works right now and how the ecosystem is using it, package.json#engines just isn't it.

[wmertens]

Given that #647 is merged, should this not be closed?

[jkrems]

@wmertens I think it's premature to close this while it causes popular libraries like angular to fail to install by default.

[wmertens]

Ok, so the issue is then that instead of erroring, it should warn? In that
case, there are like 3 duplicates of this request, it would be nice to have
that be only one canonical one, preferably with PR :)

I suppose it can be as simple as changing

yarn/src/package-compatibility.js

Line 164 in b880d40

pushError(this.reporter.lang('incompatibleEngine', name, range));

to
be like the warning a few lines down? I don't quite follow the logic in
that function, I don't know where the ignore comes from.

On Fri, Oct 14, 2016 at 9:29 PM Jan Olaf Krems [email protected]
wrote:

@wmertens https://github.com/wmertens I think it's premature to close
this while it causes popular libraries like angular to fail to install by
default.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#634 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADWluKP-laPpuCgu062fSkuGjnaVTaBks5qz9gCgaJpZM4KT1MX
.

[samccone]

@jkrems best to open a PR into angular2-cookie to resolve that modules issues.

Closing since #647 was merged.

[ghost]

Cross linking #1102