Skip to content

Conversation

liamcmitchell
Copy link

@liamcmitchell liamcmitchell commented Aug 28, 2025

Discovered while investigating #8535 (comment).
I noticed that not all deps of a failed optional dep were added to the trash list.

I modified the tests to expose this and changed the implementation to fix it.

Best illustrated with this graph showing the results of the test case const setO = optionalSet(nodeO):

image

Yellow nodes were returned by the prev implementation. Yellow + green nodes are returned by the fixed implementation.

Code to produce graph
const oldS = gatherDepSet(set, edge => !edge.optional)
const newS = gatherDepSet(set, edge => !set.has(edge.to))
if (!oldS.size !== newS.size) {
  graph(oldS, newS)
}
function graph (...sets) {
  const safe = (id) => id.replaceAll('@', '_')
  let code = 'flowchart\n'
  for (const node of sets[0].values().next().value.root.inventory.values()) {
    const rgb = sets.map(s => s.has(node) ? 'c' : '6').concat(...'666').slice(0, 3).join('')
    code += `style ${safe(node.pkgid)} fill:#${rgb},color:#000\n`
    for (const edge of node.edgesOut.values()) {
      code += `${safe(node.pkgid)}-${edge.optional ? '.' : ''}->|${edge.type} ${edge.error || ''}|${safe(edge.to?.pkgid || `${edge.name}@${edge.spec}`)}\n`
    }
  }
  const mermaid = JSON.stringify({ theme: 'neutral' })
  console.log(`https://mermaid.live/edit#pako:${require('zlib').deflateSync(JSON.stringify({ code, mermaid })).toString('base64')}`)
}

@liamcmitchell liamcmitchell requested a review from a team as a code owner August 28, 2025 14:12
@liamcmitchell liamcmitchell mentioned this pull request Aug 29, 2025
@liamcmitchell
Copy link
Author

Test failures were from missing snapshots that were deleted because of skipped tests. I restored those snapshots and rebased.

@liamcmitchell liamcmitchell changed the title fix: only filter inbound dep set edges fix: optional set calculation Aug 31, 2025
@liamcmitchell
Copy link
Author

I moved my fix from inside gatherDepSet to the filter function passed into it.

I will spend some more time looking at gatherDepSet because I think it could be improved but at least this can be of use on it's own.

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 this pull request may close these issues.

1 participant