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

L-Qun · 2024-05-11T08:25:44Z

Summary

Fix some bugs for the lockfile-explorer tool

How it was tested

Manually tested with Rushstack repo locally.

apps/lockfile-explorer/src/start.ts

iclanton · 2024-05-13T20:56:57Z

apps/lockfile-explorer/src/utils.ts

+
+/**
+ * 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


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

common/config/rush/browser-approved-packages.json

common/changes/@rushstack/lockfile-explorer/main_2024-05-11-08-08.json

iclanton · 2024-05-13T20:58:04Z

apps/lockfile-explorer-web/src/parsing/readLockfile.ts

+        if (dependency.entryId.startsWith('/')) {
+          // Local package
+          // eslint-disable-next-line no-console
+          console.error('Could not resolve dependency entryId: ', dependency.entryId, dependency);
+        }


Is the else case not an error?

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

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

The previous simple replace would cause the "@" at incorrect positions to be replaced:

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)'"

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.

octogonz · 2024-05-13T23:37:35Z

apps/lockfile-explorer/src/start.ts

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


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

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

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

octogonz · 2024-05-13T23:38:14Z

apps/lockfile-explorer/src/start.ts

-    const doc = yaml.load(pnpmLockfileText);
+    const doc = yaml.load(pnpmLockfileText) as Lockfile;
+    const { packages, lockfileVersion } = doc;
+    if (packages && lockfileVersion.toString().startsWith('6.')) {


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

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

octogonz · 2024-05-13T23:40:11Z

apps/lockfile-explorer/src/start.ts

@@ -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;


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.

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

Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>

octogonz · 2024-05-14T23:02:01Z

apps/lockfile-explorer/src/start.ts

+    if (typeof lockfileVersion === 'string') {
+      shrinkwrapFileMajorVersion = parseInt(lockfileVersion.substring(0, lockfileVersion.indexOf('.')), 10);
+    } else if (typeof lockfileVersion === 'number') {
+      shrinkwrapFileMajorVersion = lockfileVersion;


If it is encoded as a number, then shrinkwrapFileMajorVersion should probably be Math.floor(lockfileVersion) right?

octogonz · 2024-05-14T23:04:54Z

apps/lockfile-explorer/src/start.ts

+
+    let shrinkwrapFileMajorVersion: number;
+    if (typeof lockfileVersion === 'string') {
+      shrinkwrapFileMajorVersion = parseInt(lockfileVersion.substring(0, lockfileVersion.indexOf('.')), 10);


This code assumes the version contains . and that it starts with digits. Otherwise parseInt() can return NaN which will fail the test if (shrinkwrapFileMajorVersion < 5 || shrinkwrapFileMajorVersion > 6).

Generally speaking, we don't need to worry about variables not conforming to their declared TypeScript type or expected value. However in this case, the input file has not been validated with a JSON schema, and the version of its file format is also unknown. Thus some extra validation here seems worthwhile.

octogonz · 2024-05-15T02:30:18Z

@L-Qun Thanks for making these fixes! 🎉

fix some bugs for lockfile-explorer

2afaf0a

L-Qun requested review from iclanton, octogonz, apostolisms, D4N14L, dmichon-msft and patmill as code owners May 11, 2024 08:25

L-Qun added 3 commits May 11, 2024 08:54

fix some bugs for lockfile-explorer

6ecdc92

fix some bugs for lockfile-explorer

51b86d3

fix some bugs for lockfile-explorer

c3aa861

iclanton reviewed May 13, 2024

View reviewed changes

octogonz reviewed May 13, 2024

View reviewed changes

octogonz changed the title ~~fix some bugs for lockfile-explorer~~ [lockfile-explorer]: Fix some issues when parsing certain lockfile syntaxes May 13, 2024

octogonz changed the title ~~[lockfile-explorer]: Fix some issues when parsing certain lockfile syntaxes~~ [lockfile-explorer] Fix some issues when parsing certain lockfile syntaxes May 13, 2024

L-Qun and others added 6 commits May 14, 2024 11:16

Update apps/lockfile-explorer/src/start.ts

ac3e628

Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>

Merge branch 'microsoft:main' into main

3497947

fix: move '@pnpm/lockfile-types' to nonbrowser-approved-packages

2d73af3

chore: describe more specifically what's being fixed

6ec7d1c

fix: throw error if lockfile version is not supported

8de41b8

fix: move rush-lib to dependencies

e0144b3

L-Qun force-pushed the main branch from cfcf9d4 to e0144b3 Compare May 14, 2024 11:07

chore: remove useless logic

35fb928

octogonz reviewed May 14, 2024

View reviewed changes

L-Qun added 3 commits May 14, 2024 23:38

fix: resolve comments

6856ee2

fix: resolve comments

f647852

chore: rush change

5afbebc

octogonz approved these changes May 15, 2024

View reviewed changes

octogonz merged commit 624dc0c into microsoft:main May 15, 2024
5 checks passed

iclanton mentioned this pull request May 15, 2024

[lockfile-explorer] missing dependencies for first init #4705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

L-Qun commented May 11, 2024

iclanton May 13, 2024

iclanton May 13, 2024

L-Qun May 14, 2024 •

edited

octogonz May 13, 2024

L-Qun May 14, 2024 •

edited

L-Qun May 14, 2024

octogonz May 13, 2024

L-Qun May 14, 2024

octogonz May 13, 2024

L-Qun May 14, 2024

octogonz May 14, 2024

octogonz May 14, 2024 •

edited

octogonz commented May 15, 2024

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

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

Conversation

L-Qun commented May 11, 2024

Summary

How it was tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L-Qun May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L-Qun May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octogonz May 14, 2024 • edited

Choose a reason for hiding this comment

octogonz commented May 15, 2024

L-Qun May 14, 2024 •

edited

L-Qun May 14, 2024 •

edited

octogonz May 14, 2024 •

edited