fix: stop manifest search on devEngines package managers#811
Open
michkot wants to merge 12 commits intonodejs:mainfrom
Open
fix: stop manifest search on devEngines package managers#811michkot wants to merge 12 commits intonodejs:mainfrom
michkot wants to merge 12 commits intonodejs:mainfrom
Conversation
a148873 to
355d22d
Compare
…uested package manager
All code paths already had an explicit control-flow exit via return or throw, so the loop did not affect behavior.
355d22d to
1cc5eb0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #779
Summary
This branch is intentionally a small stack, not a single behavioral change.
The main fix is the manifest-selection bug from #779: when scanning parent directories,
loadSpecnow stops at the nearestpackage.jsonthat defines eitherpackageManagerordevEngines.packageManager. Before that, the scan only stopped forpackageManager, so a closer manifest using onlydevEngines.packageManagercould be skipped and a parent manifest could win instead.The branch then adds two explicit follow-up behavior changes that became clearer while fixing the lookup:
packageManager: ""andpackageManager: nullare now treated as defined but invalid values of the nearest manifest, so lookup stops there and reports an invalid"packageManager"instead of treating those values as absent and falling through to a parentpackage.jsonEngine.findProjectSpecnow honors non-erroronFailfor package-manager name mismatches when the selected project package manager comes only fromdevEngines.packageManager; in those cases,onFail: warn/ignorecan fall back to the requested package manager instead of throwingThe branch also improves invalid
"packageManager"errors so they mention which field in the selectedpackage.jsonwas used.Decision table
The table below describes the outcome for the nearest selected manifest. It intentionally ignores transparent-command behavior, which can still choose fallback for separate reasons.
Tests
devEngines.packageManagermust win over a parentpackageManagerEngine.findProjectSpeccoverage for:devEngines.packageManagerwhen the parent has no package-manager fielddevEngines.packageManagerover a parentpackageManagerdevEngines.packageManagerwithonFail: warn, which must stop the search and use fallback rather than a parentpackageManagerpackageManager: ""packageManager: nullNotes
packageManager: ""andpackageManager: null: they no longer behave like an absent field during upward lookup.onFailshould probably not affect structurally invaliddevEngines.packageManagerdefinitions, for example whennameis not a string orversionis not a valid semver range. Those look like malformed configuration rather than mismatch-policy choices, so they likely should always throw. The mismatch cases can still remain controlled byonFail.Commit ordering
packageManager: "" | nullhandling, then theonFail: warnfallback behavior, then the error-path improvement