-
Notifications
You must be signed in to change notification settings - Fork 319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Print reason when node engine does not match #1424
Changes from all commits
f19e948
0325ed2
52f59c1
d127cbd
8730143
9899d51
013d034
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import ProgressBar from 'progress' | ||
import { Index } from '../types/IndexType' | ||
import { Options } from '../types/Options' | ||
import { VersionSpec } from '../types/VersionSpec' | ||
import getPackageManager from './getPackageManager' | ||
|
||
/** | ||
* Get the engines.node versions from the NPM repository based on the version target. | ||
* | ||
* @param packageMap An object whose keys are package name and values are version | ||
* @param [options={}] Options. | ||
* @returns Promised {packageName: engines.node} collection | ||
*/ | ||
async function getEnginesNodeFromRegistry(packageMap: Index<VersionSpec>, options: Options) { | ||
const packageManager = getPackageManager(options, options.packageManager) | ||
if (!packageManager.getEngines) return {} | ||
|
||
const numItems = Object.keys(packageMap).length | ||
let bar: ProgressBar | ||
if (!options.json && options.loglevel !== 'silent' && options.loglevel !== 'verbose' && numItems > 0) { | ||
bar = new ProgressBar('[:bar] :current/:total :percent', { total: numItems, width: 20 }) | ||
bar.render() | ||
} | ||
|
||
return Object.entries(packageMap).reduce(async (accumPromise, [pkg, version]) => { | ||
const enginesNode = (await packageManager.getEngines!(pkg, version, options)).node | ||
if (bar) { | ||
bar.tick() | ||
} | ||
const accum = await accumPromise | ||
return { ...accum, [pkg]: enginesNode } | ||
}, Promise.resolve<Index<VersionSpec | undefined>>({})) | ||
} | ||
|
||
export default getEnginesNodeFromRegistry |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import { minVersion, satisfies } from 'semver' | ||
import { IgnoredUpgradeDueToEnginesNode } from '../types/IgnoredUpgradeDueToEnginesNode' | ||
import { Index } from '../types/IndexType' | ||
import { Maybe } from '../types/Maybe' | ||
import { Options } from '../types/Options' | ||
import { Version } from '../types/Version' | ||
import { VersionSpec } from '../types/VersionSpec' | ||
import getEnginesNodeFromRegistry from './getEnginesNodeFromRegistry' | ||
import upgradePackageDefinitions from './upgradePackageDefinitions' | ||
|
||
/** Checks if package.json min node version satisfies given package engine.node spec */ | ||
const satisfiesNodeEngine = (enginesNode: Maybe<VersionSpec>, optionsEnginesNodeMinVersion: Version) => | ||
!enginesNode || satisfies(optionsEnginesNodeMinVersion, enginesNode) | ||
|
||
/** Get all upgrades that are ignored due to incompatible engines.node. */ | ||
export async function getIgnoredUpgradesDueToEnginesNode( | ||
current: Index<VersionSpec>, | ||
upgraded: Index<VersionSpec>, | ||
options: Options = {}, | ||
) { | ||
if (!options.nodeEngineVersion) return {} | ||
const optionsEnginesNodeMinVersion = minVersion(options.nodeEngineVersion)?.version | ||
if (!optionsEnginesNodeMinVersion) return {} | ||
const [upgradedLatestVersions] = await upgradePackageDefinitions(current, { | ||
...options, | ||
enginesNode: false, | ||
nodeEngineVersion: undefined, | ||
loglevel: 'silent', | ||
}) | ||
const enginesNodes = await getEnginesNodeFromRegistry(upgradedLatestVersions, options) | ||
return Object.entries(upgradedLatestVersions) | ||
.filter( | ||
([pkgName, newVersion]) => | ||
upgraded[pkgName] !== newVersion && !satisfiesNodeEngine(enginesNodes[pkgName], optionsEnginesNodeMinVersion), | ||
) | ||
.reduce( | ||
(accum, [pkgName, newVersion]) => ({ | ||
...accum, | ||
[pkgName]: { | ||
from: current[pkgName], | ||
to: newVersion, | ||
enginesNode: enginesNodes[pkgName]!, | ||
}, | ||
}), | ||
{} as Index<IgnoredUpgradeDueToEnginesNode>, | ||
) | ||
} | ||
|
||
export default getIgnoredUpgradesDueToEnginesNode |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,7 @@ const fetchPartialPackument = async ( | |
fields: (keyof Packument)[], | ||
tag: string | null, | ||
opts: npmRegistryFetch.FetchOptions = {}, | ||
version?: Version, | ||
): Promise<Partial<Packument>> => { | ||
const corgiDoc = 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*' | ||
const fullDoc = 'application/json' | ||
|
@@ -58,7 +59,10 @@ const fetchPartialPackument = async ( | |
accept: opts.fullMetadata ? fullDoc : corgiDoc, | ||
...opts.headers, | ||
} | ||
const url = path.join(registry, name) | ||
let url = path.join(registry, name) | ||
if (version) { | ||
url = path.join(url, version) | ||
} | ||
const fetchOptions = { | ||
...opts, | ||
headers, | ||
|
@@ -665,6 +669,33 @@ export const getPeerDependencies = async (packageName: string, version: Version) | |
return result ? parseJson(result, { command: [...args, '--json'].join(' ') }) : {} | ||
} | ||
|
||
/** | ||
* Fetches the engines list from the registry for a specific package version. | ||
* | ||
* @param packageName | ||
* @param version | ||
* @returns Promised engines collection | ||
*/ | ||
export const getEngines = async ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function doesn't do enough to justify it being factored out. Instead, call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I did not realize it was not exported. Let's leave this as-is then. Thanks. |
||
packageName: string, | ||
version: Version, | ||
options: Options = {}, | ||
npmConfigLocal?: NpmConfig, | ||
): Promise<Index<VersionSpec | undefined>> => { | ||
const result = await fetchPartialPackument( | ||
packageName, | ||
[`engines`], | ||
null, | ||
{ | ||
...npmConfigLocal, | ||
...npmConfig, | ||
...(options.registry ? { registry: options.registry, silent: true } : null), | ||
}, | ||
version, | ||
) | ||
return result.engines || {} | ||
} | ||
|
||
/** | ||
* Fetches the list of all installed packages. | ||
* | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { Version } from './Version' | ||
import { VersionSpec } from './VersionSpec' | ||
|
||
/** An object that represents an upgrade that was ignored due to mismatch of engines.node */ | ||
export interface IgnoredUpgradeDueToEnginesNode { | ||
from: Version | ||
to: Version | ||
enginesNode: VersionSpec | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { Index } from './IndexType' | ||
import { Version } from './Version' | ||
|
||
/** An object that represents an upgrade that was ignored due to peer dependencies, along with the reason. */ | ||
export interface IgnoredUpgradeDueToPeerDeps { | ||
from: Version | ||
to: Version | ||
reason: Index<string> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import { chalkInit } from '../src/lib/chalk' | ||
import getEnginesNodeFromRegistry from '../src/lib/getEnginesNodeFromRegistry' | ||
import chaiSetup from './helpers/chaiSetup' | ||
|
||
chaiSetup() | ||
|
||
describe('getEnginesNodeFromRegistry', function () { | ||
it('single package', async () => { | ||
await chalkInit() | ||
const data = await getEnginesNodeFromRegistry({ del: '2.0.0' }, {}) | ||
data.should.deep.equal({ | ||
del: '>=0.10.0', | ||
}) | ||
}) | ||
|
||
it('single package empty', async () => { | ||
await chalkInit() | ||
const data = await getEnginesNodeFromRegistry({ 'ncu-test-return-version': '1.0' }, {}) | ||
data.should.deep.equal({ 'ncu-test-return-version': undefined }) | ||
}) | ||
|
||
it('multiple packages', async () => { | ||
await chalkInit() | ||
const data = await getEnginesNodeFromRegistry( | ||
{ | ||
'ncu-test-return-version': '1.0.0', | ||
'ncu-test-peer': '1.0.0', | ||
del: '2.0.0', | ||
}, | ||
{}, | ||
) | ||
data.should.deep.equal({ | ||
'ncu-test-return-version': undefined, | ||
'ncu-test-peer': undefined, | ||
del: '>=0.10.0', | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate that we have to call
upgradePackageDefinitions
again, since it was already called to produceupgraded
. Optimizing this probably involves larger changes to the upgrade pipeline though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...