Improves "yarn workspaces info" #5389
Conversation
…depend on each other
arcanis
force-pushed
the
yarn-workspaces-info-deps
branch
from
ee41f79 to
e7e00b9
Compare
BYK
left a comment
BYK
left a comment
There was a problem hiding this comment.
Logic looks good. Have comments about code quality and you also need to fix linting.
Approving to unblock but please address the comments before merging :)
| const workspaceDependencies = new Set(); | ||
| const mismatchedWorkspaceDependencies = new Set(); | ||
|
|
||
| for (const dependencyType of DEPENDENCY_TYPES) { |
There was a problem hiding this comment.
Isn't it better to just omit this before the loop?
There was a problem hiding this comment.
What do you mean? Duplicate the DEPENDENCY_TYPES array, but without peerDependencies?
There was a problem hiding this comment.
Yes. It seems wasteful and less readable this way to check for this specific value in every iteration.
There was a problem hiding this comment.
Yeah, maybe we could have a DIRECT_DEPENDENCY_TYPES array.
| for (const dependencyType of DEPENDENCY_TYPES) { | ||
| if (dependencyType !== 'peerDependencies') { | ||
| for (const dependencyName of Object.keys(manifest[dependencyType] || {})) { | ||
| if (Object.prototype.hasOwnProperty.call(workspaces, dependencyName)) { |
There was a problem hiding this comment.
Don't we have a shortcut for Object.prototype.hasOwnProperty.call here? Do we really need it?
There was a problem hiding this comment.
I don't think we have, there's multiple matches for this function in the codebase.
There was a problem hiding this comment.
Ah, we should make that happen. That said I'm not sure if we need to be so defensive about this. We should be able to use Object.hasOwnProperty() I guess? Or some variant of Object.keys() that only returns own properties?
There was a problem hiding this comment.
Object.keys only returns own properties, but we're calling it on a different object from the one we're dereferencing. So for example, a malicious (or a bit weird) user could add a dependency called __proto__ to one of its workspaces, and without this check we would then access workspaces.__proto__.
src/cli/commands/workspaces.js
Outdated
| if (dependencyType !== 'peerDependencies') { | ||
| for (const dependencyName of Object.keys(manifest[dependencyType] || {})) { | ||
| if (Object.prototype.hasOwnProperty.call(workspaces, dependencyName)) { | ||
| const request = manifest[dependencyType][dependencyName]; |
There was a problem hiding this comment.
expectedRange, requestedRange? Something that wouldn't confuse me with an actual HTTP request or the request module?
|
This change will increase the build size from 10.46 MB to 10.46 MB, an increase of 807 bytes (0%)
|
2 participants
Summary
Running
yarn workspaces infowill now yield information to reconstruct the workspace dependency tree. This information can then be feed to other build systems.Test plan
Updated the tests to include the new fields