-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix(core): ensure yarn patch and path versions are correct when pruning #13847
fix(core): ensure yarn patch and path versions are correct when pruning #13847
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -1,3 +1,18 @@ | |||
/** |
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.
Updates to lock file fixtures + added copy of package.json that generates it
@@ -1006,9 +1006,9 @@ Array [ | |||
exports[`lock-file mapLockFileDataToExternalNodes yarn should map lock file data to external nodes 1`] = ` |
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.
Changes caused by updated lock file fixtures
@@ -22,20 +22,20 @@ Object { | |||
|
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.
Changes influenced by updated lock file fixtures
dependencies: { | ||
'@nrwl/devkit': '15.0.13', | ||
yargs: '17.6.2', | ||
typescript: '4.8.4', |
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.
Typescript is erroneously mapped to ~4.8.2 so we want to explicitly test for this scenario
@@ -82,7 +86,7 @@ describe('yarn LockFile utility', () => { | |||
|
|||
it('should map various instances of the same version', () => { | |||
const babelCoreDependency = | |||
parsedLockFile.dependencies['@babel/core']['@babel/core@7.19.1']; | |||
parsedLockFile.dependencies['@babel/core']['@babel/core@7.20.5']; |
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.
Fixture update
@@ -46,7 +50,7 @@ describe('yarn LockFile utility', () => { | |||
|
|||
it('should parse lockfile correctly', () => { | |||
expect(parsedLockFile.lockFileMetadata).toBeUndefined(); | |||
expect(Object.keys(parsedLockFile.dependencies).length).toEqual(324); | |||
expect(Object.keys(parsedLockFile.dependencies).length).toEqual(339); |
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.
Fixture update
@@ -104,7 +108,7 @@ describe('yarn LockFile utility', () => { | |||
Object.keys( | |||
pruneYarnLockFile(parsedLockFile, YargsAndDevkitPackage).dependencies | |||
).length | |||
).toEqual(36); | |||
).toEqual(37); |
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.
Fixture update
nx: '14.7.5', | ||
'@nrwl/cli': '15.0.13', | ||
'@nrwl/workspace': '15.0.13', | ||
nx: '15.0.13', |
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.
Fixture update
@@ -154,7 +158,7 @@ describe('yarn LockFile utility', () => { | |||
}, | |||
}, | |||
}); | |||
expect(Object.keys(parsedLockFile.dependencies).length).toEqual(386); | |||
expect(Object.keys(parsedLockFile.dependencies).length).toEqual(401); |
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.
Fixture update
@@ -180,7 +184,7 @@ describe('yarn LockFile utility', () => { | |||
|
|||
it('should map various instances of the same version', () => { | |||
const babelCoreDependency = | |||
parsedLockFile.dependencies['@babel/core']['@babel/core@7.19.1']; | |||
parsedLockFile.dependencies['@babel/core']['@babel/core@7.20.5']; |
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.
Fixture update
"@nrwl/devkit": "14.7.5", | ||
"typescript": "~4.8.2", | ||
"yargs": "^17.4.0", | ||
"@nrwl/devkit": "15.0.13", |
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.
Fixture update
result[packageName] = result[packageName] || {}; | ||
result[packageName][key] = value; | ||
pruneTransitiveDependencies(dependencies, result, value); | ||
if (isBerry) { |
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.
For berry we also need to prune patch packages if exists
} else { | ||
value.packageMeta = [ | ||
value.resolution || | ||
`${packageName}@${getVersionFromPackageJson( |
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.
Version from the package.json is the one we keep in the new key
const originalPackageName = packageName.split('@patch:').pop(); | ||
const patchSuffix = value.packageMeta[0].split('#').pop(); | ||
value.packageMeta = [ | ||
`${packageName}@${getVersionFromPackageJson( |
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.
For patch we take patch name + version of the original package in package.json + patch suffix
@@ -299,7 +353,7 @@ function pruneTransitiveDependencies( | |||
[ | |||
...Object.entries(value.dependencies ?? {}), | |||
...Object.entries(value.optionalDependencies ?? {}), | |||
].forEach(([packageName, version]) => { | |||
].forEach(([packageName, version]: [string, string]) => { |
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.
Making VS Code happy
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.
LGTM
normalizedPackageJson: PackageJsonDeps, | ||
dependencies: LockFileData['dependencies'], | ||
result: LockFileData['dependencies'], | ||
isPatch = false |
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.
Could isPatch
be inferred?
const isPatch = packageName.includes(`@patch:`)
Or
const patchMatch = packageName.match(/(.+)@patch:(.+)/);
// maybe `packageName.match(/(?<pkg>.+)@patch:(.+)/)` for named group
if (patchMatch) {
const originalPackageName = patchMatch[1]; // patchMatch.groups['pkg'] for named group
//...
} else {
//...
}
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.
We could, but for performance reasons, it's easier just to pass the boolean, since we know at call whether it's a patch or not.
One comment, rest LGTM. |
When will this fix be released? I am still struggling with the error |
It will be released most likely tomorrow. |
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior
Expected Behavior
Related Issue(s)
Fixes #13835