Log failed optional platform dependencies as info.

[jacksonrayhamilton]

Fixes #11632, reducing the log level of nonfatal errors when installing optional dependencies on an unsupported platform (e.g. fsevents on GNU/Linux). Adapts the changes from #12841 to the current state of the repo and endeavors to implement the requested changes in this issue comment.

In the last two PRs that attempted to address this, it was suggested a "debug" error level be used, but it does not seem like "debug" exists as an error level in npmlog. "info" seems to have similar intent though, and it does eliminate the message by default:

~/projects/chokidar-test λ cat package.json 
{
  "dependencies": {
    "chokidar": "^1.7.0"
  }
}
~/projects/chokidar-test λ npm i
npm WARN chokidar-test No description
npm WARN chokidar-test No repository field.
npm WARN chokidar-test No license field.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.1.3 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.1.3: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

added 116 packages in 4.831s
~/projects/chokidar-test λ ../npm/bin/npm-cli.js i
npm WARN chokidar-test No description
npm WARN chokidar-test No repository field.
npm WARN chokidar-test No license field.

added 116 packages in 4.122s

[jacksonrayhamilton]

Also, it has been suggested that a more elegant enhancement to NPM would be to be able to specify that some of a package's dependencies are "platform-specific."

Even given that were true, I still think this is a worthwhile error message to make less loud in this context. Yes, in theory, it could be a useful message if someone installed and was about to immediately attempt to use a package incompatible with their platform, as the optional dependency would not be found when required. But, practically, whoever is installing the rare platform-specific package as an optional dependency probably knows what they're doing. Most people unaware of the platform-specific quality of a package would probably discover the quality as a result of installing the package as a normal dependency, as incompatible platform errors are still displayed outside the optional context. Of course, as discussed in other issues, the error mostly serves to needlessly frighten package consumers, which seems to far outweigh the fringe use case I mentioned.

Regarding describing packages as being "platform-specific," I actually prefer the current "approach" NPM has taken (intentional or not), which seems to be implied by the description of optionalDependencies:

If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies object.

The implication being: "optionalDependencies does not ask for permission; rather, it asks for forgiveness." Given that the installation of an optional dependency can include arbitrarily complex conditions for its use, which is the very tactic used by fsevents, "platform-specific" dependencies are already demonstrably implementable with no complexity and without any enhancement to package.json. Furthermore, even if conditions for use were specified in package.json: First, platform-specific restrictions would be out-of-scope for a consumer; that's the responsibility of the package itself. Second, the installation of the package could still fail for reasons outside the conditions of use. There would be an increase in complexity with no practical benefit, at least for this use case. So I disagree with that course of action.

[Adam-Pond]

Any update with this ?
Will this be available in a version soon ?

[jacksonrayhamilton]

It would be great if someone (@iarna? @zkat?) would review and merge this.

[jacksonrayhamilton]

@iarna? @zkat? Interested in this change?

[jacksonrayhamilton]

Anyone? @iarna @zkat @othiym23

I’ll accept if you’re busy, but at least say something. There are other contributions I’d like to make to NPM, but if they’re never going to even be acknowledged, why bother?

[Adam-Pond]

Good initiative Jackson. Don't give up.

…

On Feb 9, 2018 03:41, "Jackson Ray Hamilton" ***@***.***> wrote: Anyone? @iarna <https://github.com/iarna> @zkat <https://github.com/zkat> @othiym23 <https://github.com/othiym23> I’ll accept if you’re busy, but at least *say something*. There are other contributions I’d like to make to NPM, but if they’re never going to even be acknowledged, why bother? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#19198 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWbQgkRKdpzmJL7l_8KpH4DWDqDdAae_ks5tSyPPgaJpZM4QjzsD> .

[vandyw95]

@jacksonrayhamilton Hello there, I found this PR while searching for fsevents that confirmed to be a problem in my company apps. Yes the npm "Warn" us about the OS especially with the fsevents case. I think that this is quite "fatal" to change "warn" into "info" as I found out without this specific module, there are delay with functions and variables' value in our apps (Development in MacOS while deploy in Ubuntu Server). So I think that if it "Warn", there might be problems that you won't know until you find out that it would cause quite a "fatal" case. It might be good to solve it rather than skipping it.

I'll share a bit why this must be fix while it "warn". At my company, we skip this module and cause delay issues with features that I develop myself. At first I thought there might be something wrong with my code and then I refactor my code. Made a fix and push to git and deploy. (Note that I refactor my code entirely 4 times and I feel a bit burnt out and decided to check up with the devops team because I thought how could my code get wrong? wtf happened? 4 times refactor but the bug still there?) But still the delay bug won't go away until I find out there is something wrong with script in Jenkins (Automation deploy tool that we use) that it skip the optional dependencies. At last decided to push entire node_modules to git so that the Ubuntu server could build / run production mode without error.

But now I still searching for alternative to fsevents because it's not a good way to force a module from different environment or platform to be use in another one.

[jacksonrayhamilton]

@vandyw95 I don’t quite understand how skipping this dependency causes issues for you. You describe a “delay” but I don’t know what this means.

[YasharF]

The related github issues comprehensively discuss why this change needs to be implemented to address the specific case.

@vandyw95 I would suggest reading the related issues first to better grasp why this change and PR has been submitted. There is no reason to repeat what has already been discussed.

[vandyw95]

@jacksonrayhamilton So the delay I had is with the variables' value that changes while watching another functions triggered under certain condition (event).

In my case, there are several checkbox that have custom value (a JSON) and several label that watch a variable(array type) with value (using reduce function) that being pushed from the checkbox's JSON.variable every time the checkbox triggered.

Let's say there are 4 checkbox (a, b, c, and d) with value comes from an endpoint API request.
var content=[{id:1, x:1, y:1, z:1},{id:2, x:2, y:2, z:2},{id:3, x:3, y:3, z:3},{id:4, x:4, y:4, z:4}]
Check the checkbox >> Push checked value(x) into an array variable (var count) >> Label shows a Number from "count" variable by reduce it. So if I checked checkbox a and c the Label should show "4"

Delay -> When I checked checkbox a the Label shows "0" (should show "1"), when I checked b the Label shows "1" (should show "3")

Delay only happens in Production server which is Ubuntu (Developers develop with MacOS)
FYI I develop with VueJS

edit Note that the production server skip the optional because it is just a "WARN"

reducing the log level of nonfatal errors << I would say that this is a bit fatal as what I do by ignoring the "WARN" at first

@YasharF Yes as I had said earlier, why don't you read about my problem? :) Maybe there would be the same issue as what I had now

[jacksonrayhamilton]

@vandyw95 the error you describe seems to be a production client-side browser issue. The warning I want to suppress occurs during an npm install for a development-only build tool. I think you have the source of your issue confused. The presence or lack thereof of fsevents shouldn’t affect whether your checkbox state propagates. Also, your server should have no need for fsevents when building your code.

[vandyw95]

@jacksonrayhamilton FYI I'm not here mainly to discuss about fsevents but to remind and let you know that what I do to fix bug is to include that fsevents even though different environment (Yea, like what you said it shouldn't affected). But if fact, it works without changing my code. So then, how "WARN" should be lower because it shouldn't affecting things and etc. As said before, "shouldn't affecting" sounds like lack of confidence in a statement. Like I said, you won't know something is wrong until you feel the pain. :)

[RiZKiT]

@vandyw95 So to understand right, you added fsevents, which is for Node based tools to get file system events on MacOS X and added this to your Ubuntu production enviroment, to improve the performance of ajax requests!? That's kinda magic! But as long as is solves your problem, go for it, please.

I would have suggested, debug your checkbox functionality with plain javascript, maybe just in the browser console, there you can already use fetch and async/await and with a gaze at the network tab find the bottleneck.

[vandyw95]

@RiZKiT Yes it's magic and I'm a magician graduated from Hogwarts :)
Ajax has nothing to do with the checkboxes. The JSON is preloaded before the checkboxes appeared.

[welwood08]

@vandyw95 I think you have not finished debugging your problem, committing the entire node_modules directory to version control is clearly a workaround that has by coincidence included something else you need.

As has been stated, fsevents is a module specifically designed to make use of the Mac OSX native FSEvents filesystem notifications API which does not exist on Ubuntu. The module contains code which immediately throws a fatal error if it is run on a non-darwin platform, so including it on a non-darwin platform clearly can't be what has solved your problem with checkboxes in the browser and indeed if it were being used would cause you even bigger problems.

if (process.platform !== 'darwin')
throw new Error('Module \'fsevents\' is not compatible with platform \'' + process.platform + '\'');

[zkat]

Something like this is a good idea. The current model of dumping tons of errors has been a pretty painful thing at times.

I'm not going to make a call on this specific patch this week 'cause I'd like to get more of a discussion with @iarna about it and I see this as lower priority than the reams of other PRs I'm gonna be going through this week. Hang in there, though, we'll have a better response on our next pass :) (and thanks for filing this -- it's totally a legit use case!)

[0rvar]

What is there to discuss? It's crystal clear what the community wants: #11632

Just merge this.

[eric-914]

I'd like to note that once this fsevent dependency gets pulled in by any package, all further installs of future packages will result in the same warning being displayed again and again and again.

[spottedmahn]

we'll have a better response on our next pass

Hi @zkat - any updates? When might that happen? Thanks 🙏

[jacksonrayhamilton]

@spottedmahn It is, indeed, a matter of prayer.

[Amareis]

Oh, I so much like this "community-driven" development. What should we do for merge this PR? Find some collaborators and to threaten him with a gun?

[0rvar]

I give up. I just moved our company over to Yarn.

The NPM governance is just absurd.

[Amareis]

@0rvar good fix! I'll try it too.

[jacksonrayhamilton]

I agree. I understood Yarn to be an innovation, but after NPM “caught up” with lockfiles, I figured it would be wiser to stick with NPM as the “de-facto” package manager.

That was a mistake. NPM’s lack of agency is pathetic. It is so disappointing to my sense of free software idealism, that a project needs to be entirely rewritten and rebranded for the right to merge patches, as is the case with the emergence of Yarn.

SORRY NPM; I WAS WILLING TO WRITE FREE CODE FOR YOU, BUT YOU IGNORED ME, SO I WILL NO LONGER DO THAT

[legodude17]

@jacksonrayhamilton I don't think the developers are ignoring this on purpose, they probably a) haven't seen it, or b) don't have time right now.

[Amareis]

@legodude17 they need to click one button to merge this. What is a problem?

Anyway, I switch our project to yarn yesterday, so I don't care 'bout npm.

[legodude17]

@Amareis they also need to review it and test it and such.

[vlykarye]

So... Can we vote for new project collaborators with write access or something? It's been half a year.

[zkat]

Hi! We're moving repos to https://github.com/npm/cli/pulls! See our blog post about the migration to npm.community for details. As such, we're closing all active PRs on this repo.

Could you please re-open this PR against npm/cli instead? We're still interested in this patch so we hope you will! We'll continue discussion over there once it's moved. 💚

[jacksonrayhamilton]

Could you please re-open this PR against npm/cli instead?

Eh, maybe in a year or two.