jslicense / licensee.js

check dependency licenses against rules
https://www.npmjs.com/package/licensee
Apache License 2.0
185 stars 23 forks source link

Fix pnpm compatibility #82

Open CvX opened 1 year ago

CvX commented 1 year ago

@npmcli/arborist (incorrectly?) returns dependencies with no package info when looking in /node_modules/.pnpm directory.

It does also return correct data from deeper nested dirs like for example /node_modules/.pnpm/tmp@0.2.1/node_modules/tmp so we can just ignore the invalid entries.

ljharb commented 1 year ago

Is this still the case with arborist 6, which will be in v10 of licensee?

CvX commented 1 year ago

Sorry, missed that comment!

I just checked using "@npmcli/arborist": "^6.2.7" and the behavior seems the same, it does return those empty/invalid packages:

{
  "name": "licensee-playground",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "@npmcli/arborist": "^6.2.7",
    "mit-licensed": "^1.0.0"
  }
}
const Arborist = require('@npmcli/arborist');
const arborist = new Arborist();
arborist.loadActual({ forceActual: true }).then((tree) => {
  const dependencies = Array.from(tree.inventory.values()).filter(
    (dependency) => !dependency.package.name
  );
  console.log(dependencies.length); // 108
});
ljharb commented 1 year ago

Hmm - thinking about this more, the node_modules/.pnpm directory shouldn't be looked at at all anyways. If licensee were to skip folders that start with a dot, would this fix be needed?

kemitchell commented 1 year ago

Is licensee even choosing directory targets? Isn't it just calling Arborist in CWD?

Ideally we shouldn't care about npm versus yarn versus pnpm or any other thing-that-populates-node_modules. We should fire Arborist off at node_modules and go from there.

We'd have to reconsider Arborist, or using Arborist alone, if Arborist decided to support just the way npm does node_modules, but not other ways that Node.js says it can be done. I haven't seen any sign of that yet.

ljharb commented 1 year ago

That's a good point, and I agree - this may be a bug in arborist, in which case that's where it should be fixed.