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

Fix pnpm compatibility #82

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix pnpm compatibility #82

wants to merge 1 commit into from

Conversation

CvX
Copy link
Contributor

@CvX CvX commented Nov 4, 2022

@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/[email protected]/node_modules/tmp so we can just ignore the invalid entries.

@ljharb
Copy link
Member

ljharb commented Nov 16, 2022

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

@CvX
Copy link
Contributor Author

CvX commented Apr 7, 2023

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
Copy link
Member

ljharb commented Apr 7, 2023

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
Copy link
Member

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
Copy link
Member

ljharb commented Apr 9, 2023

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.

@jayvdb
Copy link
Contributor

jayvdb commented Sep 2, 2024

@ljharb , can we get this fix merged and released?

@kemitchell
Copy link
Member

@jayvdb, this was identified as an issue, if at all, with Arborist, a dependency of Licensee. Are you aware of any developments in Arborist or the use of node_modules by popular package managers, such as pnpm?

@jayvdb
Copy link
Contributor

jayvdb commented Sep 2, 2024

So far all I know is that this PR fixes the problem with using licensee with pnpm.

I dont know whether there is a bug or not in Arborist. I do know the problem in licensee with pnpm was not fixed with this commit 54a22a2

https://www.npmjs.com/package/@npmcli/arborist is now at v7.5.4

@jayvdb
Copy link
Contributor

jayvdb commented Sep 2, 2024

Updating aborist to v7.5.4 didnt fix the problem

@kemitchell
Copy link
Member

kemitchell commented Sep 2, 2024

@ljharb, I have been playing fast and loose on main to nail #93 and try and fix tests, but I'm done now. Fear not further merge conflicts.

@ljharb
Copy link
Member

ljharb commented Sep 2, 2024

No worries, was just rebasing. I’ll leave merging to you when you’re ready :-)

@kemitchell
Copy link
Member

kemitchell commented Sep 2, 2024

Sounds like fair karma.

For what it's worth, a number of deps are out of date, including Arborist. A few thoughts there:

  • We're always careful on Licensee about bumping things too readily. We offer broad Node version support, so some newer deps will break or steer us wrong. Suspect we pinned Standard for a reason, as an example.

  • We'll want to keep an eye on compat and other shenanigans from Arborist.

  • I'm still loathe to do anything pnpm-specific here in Licensee. If Arborist starts discriminating against it, we may have to think about addressing here. But I'd be looking for another package abstracting over Node's own node_modules rules, not looking to take over writing that kind of abstraction layer.

@jayvdb jayvdb mentioned this pull request Sep 2, 2024
@ljharb
Copy link
Member

ljharb commented Sep 2, 2024

a thorough CI matrix, combined with an npx ls-engines check, should ensure we don’t have to worry about engine breakage.

i 100% agree about pnpm-speciifc; either arborist should handle it, or pnpm (or yarn, etc) should fix it.

@jayvdb
Copy link
Contributor

jayvdb commented Sep 4, 2024

Firstly, this is not a bug in pnpm. It has a different layout in node_modules, specifically a .pnpm subdirectory.

Also, it does not appear to be a problem in arborist. They have copious tests covering pnpm. c.f. https://github.com/search?q=repo%3Anpm%2Fcli%20pnpm&type=code

There isnt good docs for how arborist describes pnpm/yarn managed package trees, and for that I have created npm/cli#7765

When using arborist with pnpm managed node_modules, it returns items like the following two which are correctly handled by licensee.js

ArboristLink {
  name: 'autoprefixer',
  version: '10.4.20',
  location: 'node_modules/autoprefixer',
  path: '/home/jayvdb/work/ros-frontend/node_modules/autoprefixer',
  realpath: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/autoprefixer',
  resolved: 'file:.pnpm/[email protected][email protected]/node_modules/autoprefixer',
  dev: true,
  edgesIn: Set(3) {
    { "" dev [email protected] },
    { node_modules/.pnpm/[email protected][email protected][email protected][email protected]/node_modules/lottie-react dev autoprefixer@^10.4.2 },
    { node_modules/.pnpm/[email protected][email protected]_@[email protected]_@[email protected]__@[email protected][email protected]_/node_modules/tailwindcss dev autoprefixer@^10.4.14 }
  },
  target: ArboristNode {
    name: 'autoprefixer',
    version: '10.4.20',
    location: 'node_modules/.pnpm/[email protected][email protected]/node_modules/autoprefixer',
    path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/autoprefixer',
    dev: true,
    edgesOut: Map(7) {
      'browserslist' => { prod browserslist@^4.23.3 -> node_modules/.pnpm/[email protected][email protected]/node_modules/browserslist },
      'caniuse-lite' => { prod caniuse-lite@^1.0.30001646 -> node_modules/.pnpm/[email protected][email protected]/node_modules/caniuse-lite },
      'fraction.js' => { prod fraction.js@^4.3.7 -> node_modules/.pnpm/[email protected][email protected]/node_modules/fraction.js },
      'normalize-range' => { prod normalize-range@^0.1.2 -> node_modules/.pnpm/[email protected][email protected]/node_modules/normalize-range },
      'picocolors' => { prod picocolors@^1.0.1 -> node_modules/.pnpm/[email protected][email protected]/node_modules/picocolors },
      'postcss' => { peer postcss@^8.1.0 -> node_modules/.pnpm/[email protected][email protected]/node_modules/postcss },
      'postcss-value-parser' => { prod postcss-value-parser@^4.2.0 -> node_modules/.pnpm/[email protected][email protected]/node_modules/postcss-value-parser }
    }
  }
}
ArboristNode {
  name: 'autoprefixer',
  version: '10.4.20',
  location: 'node_modules/.pnpm/[email protected][email protected]/node_modules/autoprefixer',
  path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/autoprefixer',
  dev: true,
  edgesOut: Map(7) {
    'browserslist' => { prod browserslist@^4.23.3 -> node_modules/.pnpm/[email protected][email protected]/node_modules/browserslist },
    'caniuse-lite' => { prod caniuse-lite@^1.0.30001646 -> node_modules/.pnpm/[email protected][email protected]/node_modules/caniuse-lite },
    'fraction.js' => { prod fraction.js@^4.3.7 -> node_modules/.pnpm/[email protected][email protected]/node_modules/fraction.js },
    'normalize-range' => { prod normalize-range@^0.1.2 -> node_modules/.pnpm/[email protected][email protected]/node_modules/normalize-range },
    'picocolors' => { prod picocolors@^1.0.1 -> node_modules/.pnpm/[email protected][email protected]/node_modules/picocolors },
    'postcss' => { peer postcss@^8.1.0 -> node_modules/.pnpm/[email protected][email protected]/node_modules/postcss },
    'postcss-value-parser' => { prod postcss-value-parser@^4.2.0 -> node_modules/.pnpm/[email protected][email protected]/node_modules/postcss-value-parser }
  }
}

However arborist also includes items like the following which licensee.js does not understand. This PR fixes licensee.js so that it correctly skips these entries that it doesnt need to process.

ArboristNode {
  name: '[email protected][email protected]',
  location: 'node_modules/.pnpm/[email protected][email protected]',
  path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]',
  children: Map(8) {
    'autoprefixer' => ArboristNode {
      name: 'autoprefixer',
      version: '10.4.20',
      location: 'node_modules/.pnpm/[email protected][email protected]/node_modules/autoprefixer',
      path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/autoprefixer',
      dev: true,
      edgesOut: [Map]
    },
    'browserslist' => ArboristLink {
      name: 'browserslist',
      version: '4.23.3',
      location: 'node_modules/.pnpm/[email protected][email protected]/node_modules/browserslist',
      path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/browserslist',
      realpath: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected]/node_modules/browserslist',
      resolved: 'file:../../[email protected]/node_modules/browserslist',
      dev: true,
      edgesIn: [Set],
      target: [ArboristNode]
    },
    'caniuse-lite' => ArboristLink {
      name: 'caniuse-lite',
      version: '1.0.30001651',
      location: 'node_modules/.pnpm/[email protected][email protected]/node_modules/caniuse-lite',
      path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/caniuse-lite',
      realpath: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected]/node_modules/caniuse-lite',
      resolved: 'file:../../[email protected]/node_modules/caniuse-lite',
      dev: true,
      edgesIn: [Set],
      target: [ArboristNode]
    },
    'fraction.js' => ArboristLink {
      name: 'fraction.js',
      version: '4.3.7',
      location: 'node_modules/.pnpm/[email protected][email protected]/node_modules/fraction.js',
      path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/fraction.js',
      realpath: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected]/node_modules/fraction.js',
      resolved: 'file:../../[email protected]/node_modules/fraction.js',
      dev: true,
      edgesIn: [Set],
      target: [ArboristNode]
    },
    'normalize-range' => ArboristLink {
      name: 'normalize-range',
      version: '0.1.2',
      location: 'node_modules/.pnpm/[email protected][email protected]/node_modules/normalize-range',
      path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/normalize-range',
      realpath: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected]/node_modules/normalize-range',
      resolved: 'file:../../[email protected]/node_modules/normalize-range',
      dev: true,
      edgesIn: [Set],
      target: [ArboristNode]
    },
    'picocolors' => ArboristLink {
      name: 'picocolors',
      version: '1.0.1',
      location: 'node_modules/.pnpm/[email protected][email protected]/node_modules/picocolors',
      path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/picocolors',
      realpath: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected]/node_modules/picocolors',
      resolved: 'file:../../[email protected]/node_modules/picocolors',
      dev: true,
      edgesIn: [Set],
      target: [ArboristNode]
    },
    'postcss' => ArboristLink {
      name: 'postcss',
      version: '8.4.42',
      location: 'node_modules/.pnpm/[email protected][email protected]/node_modules/postcss',
      path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/postcss',
      realpath: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected]/node_modules/postcss',
      resolved: 'file:../../[email protected]/node_modules/postcss',
      dev: true,
      peer: true,
      edgesIn: [Set],
      target: [ArboristNode]
    },
    'postcss-value-parser' => ArboristLink {
      name: 'postcss-value-parser',
      version: '4.2.0',
      location: 'node_modules/.pnpm/[email protected][email protected]/node_modules/postcss-value-parser',
      path: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected][email protected]/node_modules/postcss-value-parser',
      realpath: '/home/jayvdb/work/ros-frontend/node_modules/.pnpm/[email protected]/node_modules/postcss-value-parser',
      resolved: 'file:../../[email protected]/node_modules/postcss-value-parser',
      dev: true,
      edgesIn: [Set],
      target: [ArboristNode]
    }
  }
}

@ljharb
Copy link
Member

ljharb commented Sep 4, 2024

Having a different layout often is a bug in and of itself - altho in this case, it seems like something licensee should indeed be updated to handle.

@jayvdb
Copy link
Contributor

jayvdb commented Sep 25, 2024

@CvX , any chance you could rebase this so we can see whether CI passes now. CI is green on main.

`@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/[email protected]/node_modules/tmp` so we can just ignore the invalid entries.
@CvX
Copy link
Contributor Author

CvX commented Nov 10, 2024

All green and no conflicts again!

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

Successfully merging this pull request may close these issues.

4 participants