Add workspaces functionality to reify commands

[isaacs]

Adds support for --workspace configs in all Arborist.reify() commands, audit, and rebuild.

[isaacs]

Two issues need to be addressed in Arborist to fully finish this.

• rebuild needs to be made workspace-aware in Arborist. Right now, npm rebuild -w foo would rebuild everything regardless of where it lives in the tree. We probably want it to only rebuild the foo workspace and its dependencies, and the cli should not do tree calculus anywhere near that level.
• audit also needs to be made workspace-aware in Arborist. As it currently is, npm audit fix -w foo will only fix the vulnerabilities found in the foo workspace and its dependencies, but you'd probably expect that npm audit -w foo would only check the foo workspace and its dependencies, which it doesn't currently do.

[timoxley]

cb will never fire if getWorkspaces rejects e.g. No workspaces found${msg}

cli/lib/workspaces/get-workspaces.js

Line 30 in 6e4c1f3

throw new Error(`No workspaces found${msg}`)

Suggested change

	})
	}, cb)

[wraithgar]

The other commands use .catch instead of the second parameter error handler method. We don't really use the second parameter method for errors here (both work but consistency is also nice)

https://github.com/npm/cli/blob/latest/lib/repo.js#L36

[timoxley]

@wraithgar Sure, but if cb happens to reject/throw that will cause control to return to this .catch and then cb to fire a second time for a single invocation of execWorkspaces.

Control should never return to this function after cb is called once, and cb should never fire twice.

e.g.

function task(cb) {
   Promise.resolve().then(() => cb(null, true)).catch(cb)
}

task((err, value) => {
  console.log('task callback', err, value)
  throw new Error('oops')
})

// task callback null true
// task callback Error: oops

Using second argument to .then will turn that into an unhandled rejection, but that's probably the correct behaviour instead of calling the callback a second time when there's an error in the callback that was called and had an error. i.e. callback should not be invoked again to handle/report errors in itself.

both work but consistency is also nice

Key point is that .then(v => cb(null, v)).catch(cb) is not equivalent to then(v => cb(null, v), cb), the difference is subtle but it's zalgo territory.

The .catch handles all errors in the promise chain above it, including the .then success handler. The second argument to .then strictly only handles errors in the chain proceeding the .then, any errors in the success handler will become a rejection on promise returned by the entire chain and it's unclear if/how this should be handled.

All of the above is why mixing callbacks and promises is a pretty hairy experience overall and it should probably be entirely avoided if at all possible.

[wraithgar]

The real solution is to move off of callbacks, which is on our refactoring TODO list. For now I think we should be consistent. The callback it is being passed is our error-handler, which already has a "callback called twice" checker.

[timoxley]

The real solution is to move off of callbacks

👍

The callback it is being passed is our error-handler, which already has a "callback called twice" checker.

Sure yep, though note this is only set up for commands executed via cli, there'd be no such safety for internal calls to these methods, I don't see any so yeah perhaps a non-issue.

[darcyclarke]

@isaacs can you ensure you update the docs for each of these commands, similar to https://github.com/npm/cli/pull/3231/files#diff-9a75347976c6f12ec543901dc33c95eeca85c49b4cffae020f1cf5d9d24cc280R50-R56

[isaacs]

Alright, this is now ready for review.

[ruyadorno]

LGTM 👍

[mesqueeb]

I can't find much documentation on this, but does it mean we can now do npm i some-pkg -w=my-app ?

[wraithgar]

I can't find much documentation on this, but does it mean we can now do npm i some-pkg -w=my-app ?

The reason you don't see docs yet is because they are in this PR, which is in the release-next branch now but not yet published. e.g. https://github.com/npm/cli/pull/3227/files#diff-5d468c4e7490359f170b4de0527ea12477a429e664f7946bf311f5183ac89f5d

Once published, the parameters will show up in the -h output of all applicable commands, and also be outlined on their respective npm help and npm docs website pages.

[mesqueeb]

@wraithgar I read the docs you linked, but they're a bit vague. I want to know if it's now safe to do:

npm i some-pkg --workspace=my-sub-package

and it won't mess up the node_modules setup in my NPM 7 monorepo?

cheers!