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

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

Closed
sgielen opened this issue Nov 8, 2021 · 6 comments
Closed

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

sgielen opened this issue Nov 8, 2021 · 6 comments

Comments

@sgielen
Copy link
Contributor

sgielen commented Nov 8, 2021

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
Copy link
Contributor Author

sgielen commented Nov 8, 2021

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
Copy link
Contributor Author

sgielen commented Nov 8, 2021

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
Copy link
Contributor Author

sgielen commented Nov 8, 2021

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
Copy link
Contributor Author

sgielen commented Jan 4, 2023

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

@sgielen
Copy link
Contributor Author

sgielen commented Jan 29, 2023

@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
Copy link
Contributor Author

sgielen commented Feb 17, 2023

Fixed by #349!

@sgielen sgielen closed this as completed Feb 17, 2023
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 a pull request may close this issue.

1 participant