Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[lockfile-explorer] Fix some issues when parsing certain lockfile syntaxes #4697

Merged
merged 14 commits into from
May 15, 2024
Merged

[lockfile-explorer] Fix some issues when parsing certain lockfile syntaxes #4697

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2afaf0a
fix some bugs for lockfile-explorer
L-Qun May 11, 2024
6ecdc92
fix some bugs for lockfile-explorer
L-Qun May 11, 2024
51b86d3
fix some bugs for lockfile-explorer
L-Qun May 11, 2024
c3aa861
fix some bugs for lockfile-explorer
L-Qun May 11, 2024
ac3e628
Update apps/lockfile-explorer/src/start.ts
L-Qun May 14, 2024
3497947
Merge branch 'microsoft:main' into main
L-Qun May 14, 2024
2d73af3
fix: move '@pnpm/lockfile-types' to nonbrowser-approved-packages
L-Qun May 14, 2024
6ec7d1c
chore: describe more specifically what's being fixed
L-Qun May 14, 2024
8de41b8
fix: throw error if lockfile version is not supported
L-Qun May 14, 2024
e0144b3
fix: move rush-lib to dependencies
L-Qun May 14, 2024
35fb928
chore: remove useless logic
L-Qun May 14, 2024
6856ee2
fix: resolve comments
L-Qun May 14, 2024
f647852
fix: resolve comments
L-Qun May 14, 2024
5afbebc
chore: rush change
L-Qun May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 9 additions & 11 deletions apps/lockfile-explorer-web/src/parsing/readLockfile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,21 +139,17 @@ export function generateLockfileGraph(
for (const [dependencyKey, dependencyValue] of Object.entries(lockfile.packages)) {
// const normalizedPath = new Path(dependencyKey).makeAbsolute('/').toString();

let packageDepKey = dependencyKey;
if (pnpmLockfileVersion === PnpmLockfileVersion.V6) {
packageDepKey = dependencyKey.replace('@', '/');
}

const currEntry = new LockfileEntry({
// entryId: normalizedPath,
rawEntryId: packageDepKey,
rawEntryId: dependencyKey,
kind: LockfileEntryFilter.Package,
rawYamlData: dependencyValue
rawYamlData: dependencyValue,
subspaceName
});

allPackages.push(currEntry);
allEntries.push(currEntry);
allEntriesById[packageDepKey] = currEntry;
allEntriesById[dependencyKey] = currEntry;
}
}

Expand All @@ -171,9 +167,11 @@ export function generateLockfileGraph(
dependency.resolvedEntry = matchedEntry;
matchedEntry.referrers.push(entry);
} else {
// Local package
// eslint-disable-next-line no-console
console.error('Could not resolve dependency entryId: ', dependency.entryId, dependency);
if (dependency.entryId.startsWith('/')) {
// Local package
// eslint-disable-next-line no-console
console.error('Could not resolve dependency entryId: ', dependency.entryId, dependency);
}
Comment on lines +170 to +174
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the else case not an error?

Copy link
Contributor Author

@L-Qun L-Qun May 14, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the code, use dependencyKey and entryId to perform matching:

image

Thus, the issue "Could not resolve dependency entryId" primarily from two aspects:

  1. The previous simple replace would cause the "@" at incorrect positions to be replaced:
    image
    For example, "/@byted/xxx@1.0.0(@xxx/application-http@1.1.0)" will be converted to "//byted/xxx@1.0.0(@xxx/application-http@1.1.0)'"

  2. After being parsed by parseDependencies in LockfileDependency, the entryId is further processed based on its original content. I suppose the processed results will not appear in the pnpm-lock.yaml file. For example, content like link:../../liveart-core/packages/actions will be transformed by LockfileDependency to start with 'project', which will not appear in the pnpm-lock.yaml file. Therefore, I have imposed further restrictions here.
    image

}
}
}
Expand Down
6 changes: 4 additions & 2 deletions apps/lockfile-explorer/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@
"@types/express": "4.17.21",
"@types/js-yaml": "3.12.1",
"@types/update-notifier": "~6.0.1",
"local-node-rig": "workspace:*"
"local-node-rig": "workspace:*",
"@pnpm/lockfile-types": "~6.0.0"
},
"dependencies": {
"@rushstack/node-core-library": "workspace:*",
Expand All @@ -61,6 +62,7 @@
"express": "4.19.2",
"js-yaml": "~3.13.1",
"open": "~8.4.0",
"update-notifier": "~5.1.0"
"update-notifier": "~5.1.0",
"@pnpm/dependency-path": "~2.1.2"
}
}
14 changes: 13 additions & 1 deletion apps/lockfile-explorer/src/start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import { AlreadyReportedError } from '@rushstack/node-core-library';
import { FileSystem, type IPackageJson, JsonFile, PackageJsonLookup } from '@rushstack/node-core-library';
import type { IAppContext } from '@rushstack/lockfile-explorer-web/lib/AppContext';
import { Colorize } from '@rushstack/terminal';
import type { Lockfile } from '@pnpm/lockfile-types';

import { convertLockfileV6DepPathToV5DepPath } from './utils';
import { init } from './init';
import type { IAppState } from './state';
import { type ICommandLine, parseCommandLine } from './commandLine';
Expand Down Expand Up @@ -103,7 +105,17 @@ function startApp(debugMode: boolean): void {

app.get('/api/lockfile', async (req: express.Request, res: express.Response) => {
const pnpmLockfileText: string = await FileSystem.readFileAsync(appState.pnpmLockfileLocation);
const doc = yaml.load(pnpmLockfileText);
const doc = yaml.load(pnpmLockfileText) as Lockfile;
const { packages, lockfileVersion } = doc;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lockfileVersion is a pretty important field here. We should probably parse it into a number variable (or major/minor variables?) that can more easily be compared. And ideally we should detect format versions that are unsupported (because they are too old or too new) and in that case, display a friendly-looking error message to the end user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe lockfile-explorer tool only support v5/v6 for now. For other versions, I have implemented checks and throw an error.

if (packages && lockfileVersion.toString().startsWith('6.')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fix really about supporting multiple versions of the PNPM lockfile format? That might be a better description for the change log.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
I think the previous code already supports multiple versions of the PNPM lockfile format, although there are some issues.

const updatedPackages: Lockfile['packages'] = {};
for (const dependencyPath in packages) {
if (Object.prototype.hasOwnProperty.call(packages, dependencyPath)) {
updatedPackages[convertLockfileV6DepPathToV5DepPath(dependencyPath)] = packages[dependencyPath];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to convert V5->V6 instead of vice-versa?

Copy link
Contributor Author

@L-Qun L-Qun May 14, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the existing logic is based on handling the v5 version of path.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, I think resolving dependencies in the browser is not very appropriate, as it limits our use of pnpm-related open-source libraries.

}
}
L-Qun marked this conversation as resolved.
Show resolved Hide resolved
doc.packages = updatedPackages;
}
res.send({
doc,
subspaceName
Expand Down
15 changes: 15 additions & 0 deletions apps/lockfile-explorer/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
// See LICENSE in the project root for license information.

import * as dp from '@pnpm/dependency-path';

/**
* This operation exactly mirrors the behavior of PNPM's own implementation:
* https://github.com/pnpm/pnpm/blob/73ebfc94e06d783449579cda0c30a40694d210e4/lockfile/lockfile-file/src/experiments/inlineSpecifiersLockfileConverters.ts#L162
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you not just grab this function from a published package?

*/
export function convertLockfileV6DepPathToV5DepPath(newDepPath: string): string {
if (!newDepPath.includes('@', 2) || newDepPath.startsWith('file:')) return newDepPath;
const index = newDepPath.indexOf('@', newDepPath.indexOf('/@') + 2);
if (newDepPath.includes('(') && index > dp.indexOfPeersSuffix(newDepPath)) return newDepPath;
return `${newDepPath.substring(0, index)}/${newDepPath.substring(index + 1)}`;
}
10 changes: 10 additions & 0 deletions common/changes/@rushstack/lockfile-explorer/main_2024-05-11-08-08.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@rushstack/lockfile-explorer",
"comment": "Fix some bugs for the lockfile-explorer tool",
L-Qun marked this conversation as resolved.
Show resolved Hide resolved
"type": "patch"
}
],
"packageName": "@rushstack/lockfile-explorer"
}
4 changes: 4 additions & 0 deletions common/config/rush/browser-approved-packages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
"name": "@microsoft/rush-sdk",
"allowedCategories": [ "tests" ]
},
{
"name": "@pnpm/lockfile-types",
"allowedCategories": [ "libraries" ]
},
L-Qun marked this conversation as resolved.
Show resolved Hide resolved
{
"name": "@radix-ui/colors",
"allowedCategories": [ "libraries" ]
Expand Down