Skip to content

fix: optional set calculation #8537

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: latest
Choose a base branch
from
Open

fix: optional set calculation #8537

wants to merge 1 commit into from

Conversation

liamcmitchell
Copy link
Contributor

@liamcmitchell liamcmitchell commented Aug 28, 2025 •
edited
Loading

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
Merged
@liamcmitchell
Copy link
Contributor 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
Contributor 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.

owlstronaut pushed a commit that referenced this pull request Sep 3, 2025
@liamcmitchell
chore: fix pruner and reify tests for optional peer deps (#8540)
619d43e 
While investigating CI failure in #8537,
I found unrelated test failures when running locally on windows.

`prune with lockfile with implicit optional peer dependencies` fixed by
adding files to fixture `node_modules`, avoiding network, also reduced
package lock to minimum and removed unhelpful snapshots

`move aside symlink clutter` fixed by creating problematic symlink in
`t.testdir()` instead of in `reifyPackages` hook

---------

Co-authored-by: Liam Mitchell <liam.mitchell@mendix.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@liamcmitchell