Asymmetry between searching for closest package.json when packageManager is defined vs when devEngines.packageManager is defined after #643 #779
Description
TL;DR I believe that in #643 one part of one line of code should have been changed from !selection.data.packageManager to !parsePackageJSON(selection.data) in order to make a consistent change.
As this was not done, there is now asymmetry in lookup of "package.json that defines package manager to be used", package.json that uses packageManager is always respected, package.jsons that use devEngine.packageManager are only respected when they are top-most package.json.
I'd like a check from @aduh95 as the author of #643, I am happy to provide a PR that cleans up the current asymmetry.
In the current code (encountered / verified experimentally on v0.34.5 with DEBUG=corepack ), the algorithm defined in https://github.com/nodejs/corepack/blame/v0.34.5/sources/specUtils.ts#L166-L238, way before #643 (and #642 which was merged immediately after), loops through package.json files, starting in current directory, and stops looping & updating the selection when package.json with packageManager field is found.
Otherwise, the selection will end up the last package.json (closest to filesystem root) encountered -- and in that case, we know packageManager is not set in it either, so when the code tries again! check on it on line 237, we return 'NoSpec' and will fallback to global PM version. This bit is important.
In #643, the basic support for reading devEngines.packageManager was added, including checking it's semver against packageManager field if both are present. It was tied into the original logic by replacing the code that was reading selection.data.packageManager with a call of new function that takes both fields into account: b456268#diff-388b281fcee55f16c4f20b24ef733cc04cc2605d2e9753cffbb0ceb9ce8e6ef9L120-R193 (scroll to sources/specUtils.ts lines L120 / R 193).
However, the other critical piece, which says "look for package.json that defines package manager version to be used" , implemented in terms of "keep looping until package.json with packageManager is encountered) remained unchanged: (https://github.com/nodejs/corepack/blame/v0.34.5/sources/specUtils.ts#L173.
This is causing an asymmetry during the package.json looping/scanning. If there is a package.json with packageManager: <nonnullvalue>, the lookup will stop there. However, if the package.json contains (a valid) devEngines.packageManager field, the lookup will not stop and continue looping.
If there is no package.json above, the new feature for devEngines works "by accident", because the selection is not cleaned on each iteration, so https://github.com/nodejs/corepack/blame/v0.34.5/sources/specUtils.ts#L236 loads information from it.
However, in all other cases, the devEngines.packageManager value will be completely ignored.
e.g. in my case, this was inside a monorepo that had another package.json above it, closer to the root (it was npm monorepo package this package was was NOT part of). This other package.json was the last seen, it was stuck in selection and when fed into the rest of the function in https://github.com/nodejs/corepack/blame/v0.34.5/sources/specUtils.ts#L236 it decides the outcome. Since it did not have any PM definition, 'NoSpec' was returned it fallback to global PM.
other potential outcome is that if there would be any package.json above, with packageManager set, the lookup would stop on that file, as said, ignoring devEngines.packageManager in the first file.
Note: the root cause / logic from #560 does not seem to apply here.
P.S.: I'd go for packageManager field, but there is high change of accidentally using non-corepack-aliased-npm on this project, and npm does only care about devEngines.packageManger, so hence my choice of using this approach.
My conclusion after this little deep dive: the current behavior seems hardly desirable as it is inconsistent, and should be unified in a way that the lookup stops for packages defining either packageManager of devEngines.packageManager. There is likely missing functional test coverage for this use cases of feature #643 that could be improved. Moreover, the pre-existing implementation choice where outdated selection data is kept is bug-prone and should be refactored.