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

outdated: fix rendering for global dependencies #173

Merged
merged 1 commit into from
Mar 19, 2019

Conversation

zkat
Copy link
Contributor

@zkat zkat commented Mar 7, 2019

@zkat zkat added the semver:patch semver patch level for changes label Mar 7, 2019
@zkat zkat requested a review from a team as a code owner March 7, 2019 18:22
@aeschright aeschright merged commit d075471 into release-next Mar 19, 2019
@aeschright aeschright deleted the zkat/fix-global-outdated branch March 19, 2019 18:23
@ffxsam
Copy link

ffxsam commented Apr 1, 2019

@zkat Error is back again. npm 6.9.0.

 $ npm outdated -g
npm ERR! Cannot read property 'length' of undefined

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/samh/.npm/_logs/2019-04-01T12_45_33_869Z-debug.log

Stack:

23 verbose stack TypeError: Cannot read property 'length' of undefined
23 verbose stack     at dotindex (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/node_modules/text-table/index.js:59:32)
23 verbose stack     at /Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/node_modules/text-table/index.js:11:21
23 verbose stack     at Array.forEach (<anonymous>)
23 verbose stack     at forEach (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/node_modules/text-table/index.js:73:31)
23 verbose stack     at /Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/node_modules/text-table/index.js:10:9
23 verbose stack     at Array.reduce (<anonymous>)
23 verbose stack     at reduce (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/node_modules/text-table/index.js:63:30)
23 verbose stack     at module.exports (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/node_modules/text-table/index.js:9:20)
23 verbose stack     at /Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/lib/outdated.js:130:16
23 verbose stack     at cb (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/node_modules/slide/lib/async-map.js:47:24)
23 verbose stack     at outdated_ (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/lib/outdated.js:221:12)
23 verbose stack     at skip (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/lib/outdated.js:343:5)
23 verbose stack     at updateDeps (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/lib/outdated.js:446:7)
23 verbose stack     at tryCatcher (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/node_modules/bluebird/js/release/util.js:16:23)
23 verbose stack     at Promise.successAdapter [as _fulfillmentHandler0] (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/node_modules/bluebird/js/release/nodeify.js:23:30)
23 verbose stack     at Promise._settlePromise (/Users/samh/.nvm/versions/node/v10.15.3/lib/node_modules/npm/node_modules/bluebird/js/release/promise.js:566:21)

@jukben
Copy link

jukben commented Apr 1, 2019

Looks like the code is gone https://github.com/npm/cli/blob/latest/lib/outdated.js#L152

It looks it's gonna be fixed in 6.9.1

jukben added a commit to jukben/cli that referenced this pull request Apr 1, 2019
Looks like the code what fixed it is gone. So I reintroduce it. npm#173
@jukben jukben mentioned this pull request Apr 1, 2019
@jtsom
Copy link

jtsom commented Apr 13, 2019

Is this issue every going to be released? The latest version of node 11.14.0 includes npm v6.9.0 with this bug...

@Zearin
Copy link
Contributor

Zearin commented Apr 19, 2019

I just found out I was getting the same error, from the same operation npm [-g] outdated, but from a different function.

  • The function is located in the dependency text-table (see @substack/text-table )

  • The function itself is dotindex()

  • My crappy fix is to change the function to:

function dotindex (c) {
    var m = /\.[^.]*$/.exec(c);
    if (m) {return m.index + 1}
    else if (c !== undefined) {return c.length}
    else return 0
}

NOTE: However, I don’t know enough about npm’s architecture to say whether this is actually the correct solution. It may be that the real issue is that npm is calling the dotindex function with a bad value for c.

I’ll let the experts take it from here.

@nikoladev
Copy link

@jtsom While we wait for 6.9.1 you can install the next release, which has this fix included.

npm i -g npm@6.9.1-next.0

@Zearin
Copy link
Contributor

Zearin commented Apr 25, 2019

@nikoladev Thanks! That is awesome :D

@Orel-A
Copy link

Orel-A commented May 1, 2019

Why isn't it in @latest yet?

@jtsom
Copy link

jtsom commented May 22, 2019

Hello? Anyone home? Yet another node release (12.3) that includes the broken 6.9.0. What's the hold up?? Having to remember to reinstall a .next.0 version of NPM every time is not ideal.

@Zearin
Copy link
Contributor

Zearin commented May 25, 2019

I’ve been wondering the same thing! So much work has gone into npm, and now I’m afraid that it’s just going to let yarn (ick! 🤢) roll in and take over. :(

@Zearin
Copy link
Contributor

Zearin commented Jun 1, 2019

This fix was merged on 2019-03-20.

Why hasn’t it been published as a SEMVER-PATCH release yet?

@volschin
Copy link

volschin commented Jun 2, 2019

node 10.16 LTS is now also broken, beeing shipped with npm 6.9.0. Please take action.

@aeschright
Copy link
Contributor

Hi everyone, here’s an update on what’s been happening with the open PRs https://gist.github.com/aeschright/8ed09cbc2a4aee00fcb4ad35086d76a6

@Zearin
Copy link
Contributor

Zearin commented Jun 27, 2019

@aeschright Thanks for writing that note. I am so sorry to hear the news.

I really hope the company gets its head back on straight.

@ZuBB
Copy link

ZuBB commented Jun 27, 2019

Even with 6.9.1 I have next issue and it looks quite similar to one you fixed here

Screen Shot 2019-06-27 at 19 14 35

Does anyone have any hints on this?

@brimarq
Copy link

brimarq commented Jun 27, 2019

@ZuBB I was having the same "Cannot read property 'length' of undefined" error when running npm outdated -g with v6.9.0. I just installed v6.9.1 and it works now (at least, for that command). FWIW, I'm using node v10.15.3 installed via nvm on Darwin 17.7.0.

@stanleyxu2005
Copy link

stanleyxu2005 commented Jun 30, 2019

@zkat I've just applied your change with the shipped npm v6.9.0 in nodejs v12.5.0. The error message is gone, but the column "Location" shows "global". I expected it to be path/to/npmpkgs.

I would suggest to improve your PR by changing to

// outdated.js L148-153
  var columns = [ depname,
    has || 'MISSING',
    want,
    latest,
    deppath || npm.dir
   // or 
   // deppath || `(global) ${npm.dir}`
  ]

Follow-up Question 1

During the troubleshooting, I see one place potential to be the root cause.

// outdated.js L216-222
function outdated_ (args, path, tree, parentHas, depth, opts, cb) {
  if (!tree.package) tree.package = {}
  if (path && moduleName(tree)) path += ' > ' + tree.package.name
  if (!path && moduleName(tree)) path = tree.package.name   // <---- name set to be path??
  if (depth > opts.depth) {
    return cb(null, [])
  }

The function moduleName looks quite wired to me

// utils/module-name.js
var unknown = 0
function moduleName (tree) {
  if (tree.name) { return tree.name }
  var pkg = tree.package || tree
  if (isNotEmpty(pkg.name) && typeof pkg.name === 'string') return pkg.name.trim()
  var pkgName = pathToPackageName(tree.path)
  if (pkgName !== '') return pkgName
  if (tree._invalidName != null) return tree._invalidName
  tree._invalidName = '!invalid#' + (++unknown)
  return tree._invalidName
}

If name is invalid, the function will return tree._invalidName,

  1. so that !path && moduleName(tree) will be true,
  2. so that path = tree.package.name.

Regarding this logic, the variable path could be assigned to an unexpected value, couldn't it?
I noticed the change was introduced by this commit b7b54f2#diff-3d20499d58f14c6f1edfe93d8ba8a8a2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.