fix: refactor node.ideallyInert to node.inert

[liamcmitchell]

Discovered while investigating #8535 (comment)

Similar to #8566, relates to #8184

Moves inert (uninstallable optional) calculation into buildIdealTree so it can be used in diff.

Also removes most of #8184 in favor of a simpler fix, see PR comments for the journey.

Improvements:

• we don't see uninstallable packages as "installed" in CLI output
• createSparseTree no longer creates dirs that will only be cleaned later

For the example in the linked issue, it changes output from added 156 packages to added 12 packages and combined with #8537 it changes to added 6 packages, the expected result.

[liamcmitchell]

The test failures are in two cases of reifying with omit: optional and optional platform specific dep fsevents.

It shows a difference in how omitted and failed deps are represented in the "actual" tree that is saved to the hidden lockfile and returned from reify.

• Omitted deps are removed from the actual tree
• Failed optional deps are preserved in the actual tree with ideallyInert

I don't think this difference should exist. The next question is, do they belong in the actual tree or not?

I don't think they need to be. #8184 could also have been solved by checking for missing optional deps alongside invalid when building the ideal tree from lockfile. That check might still be needed regardless.

Omitting from actualTree and lockfile would however require recalc of omitted and failed deps on every ideal tree build. If the intention of the hidden lockfile is to avoid this, then better to save both.

[liamcmitchell]

I don't think they need to be. #8184 could also have been solved by checking for missing optional deps alongside invalid when building the ideal tree from lockfile. That check might still be needed regardless.

This solution was already suggested here: #8127 (comment)

Omitting from actualTree and lockfile would however require recalc of omitted and failed deps on every ideal tree build. If the intention of the hidden lockfile is to avoid this, then better to save both.

Rebuilding ideal tree should only happen when there is no package lock, so relatively infrequently. As to the intention of the hidden lockfile, it looks like it was always intended to represent the actual tree, not ideal. Adding failed optional deps came with #8184.

I think there is value in preserving omitted and failed deps in the actual tree, marked as inert or whatever but probably more work...

[liamcmitchell]

• removed most changes introduced by fix(arborist): omit failed optional dependencies from installed deps #8184
• fixing missing optional deps as described above was a one-line fix: https://github.com/npm/cli/pull/8602/files#diff-02626074e1a4a170693607e4a3a69dfc08ee52067734717833b22cf162923e07R354
• removed a few tests that didn't make sense to me
• converted a few snapshots to simpler expectations
• renamed ideallyInert to inert
• refactored omitted calc to also use inert

[wraithgar]

We normally do not allow changes to the package-lock.json file. It makes sense that this change is here now given that this is an optional dep of chokidar.

In order that we don't complicate things I removed the package-lock.json from my local copy of this branch and ran npm run resetdeps to verify that what's in here now is valid. The package lock was identical, so we're good to allow it in this PR as changed.

[liamcmitchell]

I only updated this after CI failed complaining about mismatched package lock. This is the result of fixing missing optional deps here: https://github.com/npm/cli/pull/8602/files#diff-02626074e1a4a170693607e4a3a69dfc08ee52067734717833b22cf162923e07R354

[wraithgar]

Landing this is going to be a real good test of our recent fixes to not prune optional deps from the package-lock, when CI runs on windows and linux.

[wraithgar]

Suggested change

	if (node && node.root === this.idealTree) {
	if (node?.root === this.idealTree) {

[wraithgar]

After a quick review the code changes seem to make sense. The deletion of tests of course always makes me nervous so I'll probably want to think on those a little more to be sure they weren't somehow load bearing.

[liamcmitchell]

After a quick review the code changes seem to make sense. The deletion of tests of course always makes me nervous so I'll probably want to think on those a little more to be sure they weren't somehow load bearing.

I added comments explaining the deletions. Hope it makes sense.

[wraithgar]

Hope it makes sense.

This helps so much!