Print reason when node engine does not match #1424
Conversation
|
@raineorshine would appreciate your CR |
… print it out, just like we do in peer checks raineorshine#1422
raineorshine
changed the title
src/types/PackageFile.ts
Outdated
| engines?: Index<VersionSpec> | ||
| engines?: Index<Version | undefined> |
There was a problem hiding this comment.
I don't think this is correct. If a package does not define a node engine, it should be omitted from the engines object. Explicit undefined should not be allowed.
There was a problem hiding this comment.
VersionSpec (i.e. a version range) is correct, not Version (an exact version).
There was a problem hiding this comment.
If you don't put undefined, the any access to the engines object will produce a type of VersionSpec which is not correct... engines.foo is not a string, but a sring | undefined this is just more safe, so when I access engines.node I will know to make sure it is not undefined.
There was a problem hiding this comment.
I think I hear what you are saying, but I don't see any indication that explicit undefined, i.e. engines: { node: undefined } }, is part of the manifest spec. The purpose of the PackageFile type is to represent the expected type of a package.json file. If there is no engine for a given key, then the key should be omitted from the engines object.
There was a problem hiding this comment.
Lets agree to disagree :-) changed
| if (!options.nodeEngineVersion) return {} | ||
| const optionsEnginesNodeMinVersion = minVersion(options.nodeEngineVersion)?.version | ||
| if (!optionsEnginesNodeMinVersion) return {} | ||
| const [upgradedLatestVersions] = await upgradePackageDefinitions(current, { |
There was a problem hiding this comment.
It's unfortunate that we have to call upgradePackageDefinitions again, since it was already called to produce upgraded. Optimizing this probably involves larger changes to the upgrade pipeline though.
There was a problem hiding this comment.
Yes I first tried to see how I can overcome it, but it requires major changes. If we ever do go ahead and make those changes, we can also not run the upgrades again for the ignored upgrades due to peer dependencies...
… print it out, just like we do in peer checks raineorshine#1422
… print it out, just like we do in peer checks raineorshine#1422
… print it out, just like we do in peer checks raineorshine#1422
|
@raineorshine would appreciate your CR |
| * @param version | ||
| * @returns Promised engines collection | ||
| */ | ||
| export const getEngines = async ( |
There was a problem hiding this comment.
This function doesn't do enough to justify it being factored out. Instead, call fetchPartialPackument directly.
There was a problem hiding this comment.
call it from where? fetchPartialPackument is internal to npm implementation, and getEngines is part of the package managers interface that is being used in the business logic. Where do you suggest I inline it and how?
There was a problem hiding this comment.
Good point, I did not realize it was not exported. Let's leave this as-is then. Thanks.
… print it out, just like we do in peer checks raineorshine#1422
… print it out, just like we do in peer checks raineorshine#1422
Fix: #1422