Harmonize package-tree analysis with and without `--production`

[amonks]

In #48, @nzacca and @kemitchell discussed the possibility of ignoring peer dependency errors.

@kemitchell said,

@nzacca ideally, I think licensee would succeed if it can do its job, and fail only if it can't, independent of whether other tools succeed or fail.

When npm ls encounters a peer-dependency violation or other error, it does three things:

• log the error to stderr
• exit with a non-zero code
• log the result normally to stdout

WHAT WAS CHANGED:

This commit adds a flag, --ignore-npm-errors that makes licensee proceed normally with the logged result, even if npm ls exits with a non-zero code.

RISK:

I'm suspect this could produce operational errors down the line, if there are cases where npm ls does not log a result to stdout.

[amonks]

This needs a test and maybe some cleanup (see RISK above), but I'd be happy to do those things if you're comfortable with the spirit of this change. 😄

Big fan of your blogs btw!

[kemitchell]

Can we ignore all npm errors that lead to nonzero exit codes, or just peer dependency errors?

A step back: Has this been an issue in the wild?

[amonks]

TLDR

• This ignores all errors that lead to nonzero exit codes. We could be more specific by matching against the strings printed to stderr.
• This has been an issue for me in the wild, but if it is not an issue for others I am happy to continue using this fork.

---

When I run npm ls --production --json in the package I'm interested in, I get errors of three forms:

npm ERR! peer dep missing: peer-package@^1.0.0, required by old-package@1.0.0
npm ERR! missing: transient-package@latest, required by rude-package@1.0.0
npm ERR! invalid: my-forked-package@1.0.0 /home/my/project/my-forked-package

• The peer dep missing errors are because the old-package has a peer dependency requirement that is too specific. old-package declares that it works with ^1.0.0 of peer-package, but it also works with 2.0.0, which fixes a bug I care about. old-package has not been updated.
• The missing errors, I suspect, are because transient-package has been updated since I installed, so I am no longer using "latest".
• The invalid error I'm not sure about. My package.json specifies my-forked-package as "my-forked-package": "githubname/my-forked-package#branchname", but I don't know if that pattern always produces an "invalid" error or if there's something else going on.

In all cases, I could take steps on my end to "fix" the error, like installing different versions of packages, or forking them to have different dependency specifications. I don't want to do this. I have good integration tests and my build works fine. Licensee.js also works fine, given this patch.

---

One check that could be added is "does every line in stderr match a pattern that we know still produces reasonable json output?"

Another check that could be added is "does every line in stderr match some explicit user-configured whitelist pattern?"

[ljharb]

If it actually works with v2 but declares it needs v1, your dep graph is invalid with v2, so that’s a real error - i hear that you don’t want to correct your dep graph, and perhaps licensee could be made to still work in that case, but it seems to me like it’s trying to cure the symptoms instead of the disease.

Have you tried forking old-package as well and updating its peer dep range, so that you don’t have to have those errors at all?

[amonks]

@ljharb Yes, I could make the error go away by forking old-package and rude-package. I probably even should do this.

But fundamentally, I think it is valid to ask the question "under what licenses am I using my dependencies" even if my dependency graph is invalid. These seem to me like orthogonal issues.

[kemitchell]

One check that could be added is "does every line in stderr match a pattern that we know still produces reasonable json output?"

Another check that could be added is "does every line in stderr match some explicit user-configured whitelist pattern?"

Do we need to look at stderr? I believe licensee currently only parses stdout

[kemitchell]

By the by, I think everyone here is "right". I agree with @amonks that if npm ls gives a usable tree, licensee could say whether its license meet the rules or not. But I also agree with @ljharb that we shouldn't go out of our way too far to make project problems licensee problems.

[amonks]

@kemitchell As far as I am aware, stderr is the only place npm ls tells us which errors happened. The change I'm thinking about is something like,

diff --git a/index.js b/index.js
index 451ec71..9a5db0c 100644
--- a/index.js
+++ b/index.js
@@ -83,16 +83,26 @@ function licensee (configuration, path, callback) {
     )
     var outputError
     var json
+    var errorIsUnworkable
     simpleConcat(child.stdout, function (error, buffer) {
       if (error) outputError = error
       else json = buffer
     })
+    simpleConcat(child.stderr, function (error, buffer) {
+      if (error) outputError = error
+      else {
+        var errorLines = buffer.toString().split('\n')
+        if (errorLines.some(l => !errorMatchesPatternWeAreOkWith(l))) errorIsUnworkable = true
+      }
+    })
     child.once('error', function (error) {
       outputError = error
     })
     child.once('close', function (code) {
       if (code !== 0 && !configuration.ignoreNpmErrors) {
         done(new Error('npm exited with status ' + code))
+      } else if (configuration.ignoreNpmErrors && code !== 0 && errorIsUnworkable) {
+        done(new Error('npm produced an unacceptable error and exited with status ' + code))
       } else if (outputError) {
         done(outputError)
       } else {

[edited] [untested but you get the idea]

[kemitchell]

How about, with the new flag enabled:

1. Note the exit code of npm ls. If it's non-zero, report that to stderr, but don't bail.

2. Ignore npm ls error output entirely.

3. If we can parse the JSON output of npm ls, process it.

[amonks]

I'll make that change. Thanks!

[kemitchell]

Maybe a slightly different flag for that approach, like --with-npm-errors.

I don't mean to nitpick or micromanage. I'm just thinking about the pressure SemVer puts to get the interface/API right the first time.

[amonks]

I learned while testing this that I was mistaken. stderr is not the only place that npm ls reports errors. It also includes a key problems in the json printed to stdout. Since we were not going to read stderr anyway, I will also not look at the problems value. Happy to do so if you'd like, though.

{
  "name": "with-npm-errors",
  "problems": [
    "missing: with-invalid-dependency@^1.0.0, required by with-npm-errors@",
    "missing: mit-licensed@^1.0.0, required by with-npm-errors@"
  ],
  "dependencies": {
    "with-invalid-dependency": {
      "required": "^1.0.0",
      "missing": true
    },
    "mit-licensed": {
      "required": "^1.0.0",
      "missing": true
    }
  }
}

[amonks]

I also learned that since licensee only invokes npm ls if configuration.production is set, licensee always ignores these sorts of package integrity errors without that flag.

So, with the change I've submitted, the behavior would be,

	--with-npm-errors	--with-npm-errors unset
--production	ignore dependency graph errors	fail on dependency graph errors
--production unset	ignore dependency graph errors	ignore dependency graph errors

currently on master, the behavior is

	always
--production	fail on dependency graph errors
--production unset	ignore dependency graph errors

---

I think this is confusing—I don't know that users would expect that the production flag would have anything to do with dependency graph integrity checking. In my opinion, there are two good ways and one mediocre way to handle this:

1. add explicit flag to check dependency graph integrity
   • add an npm ls check even with --production disabled, so licensee exits 1 if there are dependency graph errors and production is disabled
   • introduce a --with-npm-errors flag to ignore dependency graph integrity checking
2. change --production behavior so that licensee never checks dependency graph integrity
   • introduce the behavior from this PR without a flag
3. rename --with-npm-errors to something that is explicit that it depends on --production. I don't have a good suggestion.

[kemitchell]

Nice catch! Of the options you laid out, my favorite is:

change --production behavior so that licensee never checks dependency graph integrity

Simple. Simple. Simpler.

PS: @amonks, this is a lot of good work, on a Friday, after a US federal holiday. Do you have good support for all this work that you're doing? If not, let's make sure you do. E-mail me separately if you prefer to discuss privately. kyle@kemitchell.com

[amonks]

I updated the PR to the proposed change, and added a test that failed before this change.

In conclusion,

• If npm ls exits nonzero, we will log the exit code to stderr, but keep going.
• We will trust the cannot interpret npm ls --json output below to tell us if it is safe to proceed.

---

While this work on licensee.js is my own and not conducted on behalf of my employer, I am plenty well supported.

You can consider this PR a thank you for /dev/lawyer, license zero, Blue Oak, jslicense, and all the rest 😄

[kemitchell]

@amonks, do you have written permission from your employer to contribute this PR under Apache 2.0 terms?

[ljharb]

Perhaps the PR title should be updated as well?

[kemitchell]

@amonks, if the licensing situation is green, you should add yourself to package.json, too.

[amonks]

[still here; working on getting written confirmation of open source licensing policy; will make suggested changes when that lands]

[kemitchell]

@amonks thanks!

[rowanoulton]

👋 Hello! We're running into the behaviour this PR would fix, and would love to see it merged.

@amonks did you ever get confirmation from your employer?

Thank you all

[kemitchell]

Hi, @rowanoulton. Thanks for following up with @amonks.

[rowanoulton]

@kemitchell I don't want to steal @amonks's thunder, but if we don't hear back from them is this something you or I could commit to the project in their stead?

[kemitchell]

@rowanoulton we don't have a clear license for @amonks' work yet. We can't merge without one.

Worse comes to worst, we have to drop this and clean room the functionality.

[amonks]

hi! still alive! still working on an outbound open source policy, which is taking… more legal rigamarole than I expected.

in hindsight, that was too big a project to block this on, so I'll seek one-off permission for this change.

sorry for the wait!

[Whoaa512]

Any update on this? I'd really like to have this in the mainline release instead of using a fork.

[amonks]

I got one-off legal permission. I'll make the suggested changes tonight, then merge.

Thanks for your patience, all!

[kemitchell]

@amonks thank you for making sure this code is safe to share!

[amonks]

I made the suggested error message change and rebased.

---

Pending licensing confirmation, this LGTM, with one nit: We should git add --force tests/with-npm-errors-in-production/package-lock.json.

It looks like the package-lock.json is never created, perhaps because cd tests/with-npm-errors-in-production && npm install (intentionally) fails. I think this is probably fine.

> cd tests/with-npm-errors-in-production/; and npm install
npm ERR! code ETARGET
npm ERR! notarget No matching version found for mit-licensed@^2.0.0.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.
npm ERR! notarget
npm ERR! notarget It was specified as a dependency of 'with-invalid-dependency'
npm ERR! notarget

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/ajm/.npm/_logs/2020-05-16T18_17_26_851Z-debug.log

---

One more note,

In hindsight, "mit-licensed": "^2.0.0" might not be the most robust invalid dependency. I don't think you're likely to push a version 2.0 of mit-licensed, but if you did, with-invalid-dependency would no longer have an invalid dependency. It may be safer to create a new package explicitly for this purpose.

---

I'm happy to merge as is or to make further changes. Since @kemitchell I'm not following your nit about package-lock.json, I will hold off on merging until you give me a 👍.

[kemitchell]

@amonks, add yourself to package.json and let me know? I will merge, deal with the testing thing, and release.

Forgive me if I've just forgotten: Did we decide whether this is SemVer-major, -minor, or -patch?

[ljharb]

Since it errors in fewer cases, it seems like it could be a patch?

[amonks]

@amonks, add yourself to package.json and let me know? I will merge, deal with the testing thing, and release.

done.

Forgive me if I've just forgotten: Did we decide whether this is SemVer-major, -minor, or -patch?

I don't think we did.

[kemitchell]

I'm going to release as a patch bump. It might be right. It might be wrong. It's better than thinking about SemVer for an hour.

[kemitchell]

v8.0.2 out

[rowanoulton]

Ya'll the best. Thanks ❤️