fix: check if upgrade ignored due to engines fails due to usage of ranges #1447
Conversation
| async function getEnginesNodeFromRegistry(packageMap: Index<VersionSpec>, options: Options) { | ||
| async function getEnginesNodeFromRegistry(packageMap: Index<VersionResult>, options: Options) { |
There was a problem hiding this comment.
The function only uses version, so why pass in more information than what is used?
| const enginesNodes = await getEnginesNodeFromRegistry(upgradedLatestVersions, options) | ||
| const enginesNodes = await getEnginesNodeFromRegistry(latestVersionResults, options) |
There was a problem hiding this comment.
Can you explain this change for me?
There was a problem hiding this comment.
upgradedLatestVersions contains specs (^4.0.0) which can't be used to get the info from npm registry. So we use the actual versions instead
There was a problem hiding this comment.
First, in that case, let's change the first param of getEnginesNodeFromRegistry to use Version instead of VersionSpec.
i.e.:
async function getEnginesNodeFromRegistry(packageMap: Index<Version>, options: Options)(Eventually I hope to change Version to a branded type so that the type system can enforce the distinction between an exact version and a version range.)
Second, latestVersionResults is not filtered the way that upgradedLatestVersions is. Is that desired? I'm thinking we probably don't need to look up the engines node version for dependencies that have already been filtered out due to the minimal or filterResults options. That will just result in extra HTTP requests that will get thrown away.
This filters down latestVersionResults to just the keys in upgradedLatestVersions. You could pass filteredLatestVersionResults to getEnginesNodeFromRegistry instead:
const filteredLatestVersionResults = pickBy(latestVersionResults, (spec, dep) => upgradedLatestVersions[dep])There was a problem hiding this comment.
I already changed getEnginesNodeFromRegistry to use Version.
Yes, branded types might be the way thought it comes with annoying boilerplate :-(
Yes you are correct, I applied a fix
@raineorshine this is a show stopper for us, I would appreciate your input