amio / npm-why

Identifies why a package has been installed. Equivalent to `yarn why` for npm packages.
171 stars 5 forks source link

ERROR Cannot read properties of undefined (reading 'length') #269

Closed sgielen closed 1 year ago

sgielen commented 2 years ago

I get this error when trying to run npm-why in my project.

The initial reason for this error is in npm-why's main, which hides the actual error:

async function main (dir, packageName) {
  const reasons = await collectReasons(dir, packageName).catch(e => {
    if (e.code === 'ENOLOCK') {
      throw new Error('package.json or lockfile not found.')
    }
  })

  if (!reasons.length) { // <-- error is thrown here

The actual error can be shown by changing this to:

async function main (dir, packageName) {
  const reasons = await collectReasons(dir, packageName).catch(e => {
    if (e.code === 'ENOLOCK') {
      throw new Error('package.json or lockfile not found.')
    } else {
      throw e;
    }
  })

  if (!reasons.length) {

I suggest to make this change upstream, because then npm-why shows the actual error:

  ERROR Maximum call stack size exceeded

See also #220.

This seems to be because of a recursive dependency, I'm investigating further.

sgielen commented 2 years ago

It's because of a circular dependency that's easily reproduced in an empty directory:

npm install --save-dev eslint
npm-why eslint

There is an edge from eslint to eslint-utils, and from eslint-utils back to eslint, and this circular dependency eventually causes npm-why to encounter a "maximum call stack size exceeded".

sgielen commented 2 years ago

In buildDependentsTree we can detect this circular dependency and prevent further recursion, while still reporting the circular dependency. Here's some example code that does this, but I'm sure there's a prettier solution:

function buildDependentsTree (node, paths = [], leafs = [], depth = true) {
  const { name, version, edgesIn } = node

  // when leaf node
  if (edgesIn.size === 0) {
    leafs.push(paths)
    return { name, version, leafs }
  }

  // prevent dead loops
  if (paths.includes(name)) {
    return { name, version, leafs }
  }

  let dependents = []
  if (depth) {
    dependents = Array.from(edgesIn).map(edge => {
      const pkg = { name, version }
      return buildDependentsTree(edge.from, paths.concat(pkg), leafs, !paths.map(p => p.name).includes(name))
    })
  }

  return { name, version, leafs, dependents }
}

With this, the output of the reproduction above becomes the following, which clearly shows the recursion:

$  npm-why eslint

  Who required eslint:

  2021-11-08-11-05-29 > eslint@8.2.0
  2021-11-08-11-05-29 > eslint > eslint-utils > eslint@8.2.0
sgielen commented 2 years ago

Actually, I see now that the "prevent dead loops" check was intended to fix this, but it doesn't work, since paths contains {name, version} structs and not just name strings. So this also fixes the issue:

   // prevent dead loops
-  if (paths.includes(name)) {
+  if (paths.map(p => p.name).includes(name)) {
     return { name, version, leafs }
   }

However, it does not show the recursion in the output:

$ npm-why eslint

  Who required eslint:

  2021-11-08-11-36-29 > eslint@8.2.0
sgielen commented 1 year ago

@amio any updates perhaps to the two improvements suggested in this issue?

sgielen commented 1 year ago

@amio I've added #349, which includes a fix for this issue, a test for that, a fix for the issue hiding the actual exception.

sgielen commented 1 year ago

Fixed by #349!