Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Attempt exposing isDirect and types #301

Closed
wants to merge 3 commits into from
Closed

Attempt exposing isDirect and types #301

wants to merge 3 commits into from

Conversation

naugtur
Copy link
Contributor

@naugtur naugtur commented Jul 9, 2021

This is in relation to
npm/rfcs#372

Trying to fulfill the first 2 checkboxes from my list here
npm/rfcs#399

Questions:
Isn't there a nicer way to figure out a direct dependency? I'm not sure if ==='' will work with workspaces, saw examples in tests where the edge was from package/a instead of empty string.
Is it ok to add types? They're simply there so I don't have to explore other options outside audit output.

@naugtur
Copy link
Contributor Author

naugtur commented Jul 9, 2021

@isaacs @darcyclarke I can see this is fr from ready, but I'm not sure if I'm even going the right direction.

lib/vuln.js Outdated Show resolved Hide resolved
@naugtur naugtur marked this pull request as ready for review July 11, 2021 14:19
@naugtur
Copy link
Contributor Author

naugtur commented Jul 11, 2021

nyc claims my arrow functions are not covered, which seems like a bug.
Screenshot 2021-07-11 at 16-21-42 Code coverage report for lib vuln js

Other than that, I think the PR is ready.
Should I add ignore comments or is my understanding of coverage wrong?

Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main thing is clarifying the semantics around how we set types and isDirect, to ensure that we're getting the correct intended result.

As for the test coverage, the issue is that there are no cases where it's setting anything in types or setting isDirect to true, so we do need a test case to exercise that logic.

Comment on lines +97 to +99
const edgesIn = [...this.nodes.values()].flatMap(n => [...n.edgesIn])
const isDirect = edgesIn.some(e => e.from.isProjectRoot)
const types = [...new Set(edgesIn.map(a => a.type))].sort()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A node might be "optional", but not have any optional edgesIn. Or it might have optional edgesIn, but not actually be optional.

For example:

root -> (a, b) OPTIONAL(c)
a -> OPTIONAL(d)
b -> (d)
c -> (e)

In this dep graph,

  • a is not an optional dep, but has an optional dep on d
  • but b has a production dep on d, so d is not optional
  • c is an optional dep of the root project, and nothing else, so it's optional.
  • e is a production dep, but a production dep of an optional dep, so it is optional.

Is the intention to show all flags on any nodes in the set, or all nodes in the set? Eg, if there's one node in the tree that is optional, and another that isn't, should types include 'optional'?

Suggested change
const edgesIn = [...this.nodes.values()].flatMap(n => [...n.edgesIn])
const isDirect = edgesIn.some(e => e.from.isProjectRoot)
const types = [...new Set(edgesIn.map(a => a.type))].sort()
// this will add the flag to `types` if _any_ dep has the flag set
const allTypes = ['dev', 'devOptional', 'optional', 'peer']
const types = new Set()
// if the semantic should be _all_ deps must have it, do this instead:
// const types = new Set(allTypes)
let isDirect = false
for (const node of this.nodes.values()) {
for (const t of allTypes) {
if (node[t])
types.add(t)
// if the semantic should be _all_ deps must have the flag set, do this instead:
// if (!node[t])
// types.delete(t)
}
// it's a direct dep if the root project or any workspace depends directly on it
// since workspace projects are "ours" in the same sense as the root project
// only need to check if we haven't already found one.
if (!isDirect) {
for (const { from } of node.edgesIn) {
if (from.isProjectRoot || from.isWorkspace) {
isDirect = true
break
}
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually only needed dev, so I'm reluctant to include this much complexity here for figuring out types.
I'm going to split this PR so I can finish isDirect and get it merged and use it, then work on types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted here: #323

@isaacs
Copy link
Contributor

isaacs commented Sep 28, 2021

Closed by #323

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants