-
Notifications
You must be signed in to change notification settings - Fork 587
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
[lockfile-explorer] Fix some issues when parsing certain lockfile syntaxes #4697
Conversation
apps/lockfile-explorer/src/utils.ts
Outdated
|
||
/** | ||
* 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 |
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.
Can you not just grab this function from a published package?
common/changes/@rushstack/lockfile-explorer/main_2024-05-11-08-08.json
Outdated
Show resolved
Hide resolved
if (dependency.entryId.startsWith('/')) { | ||
// Local package | ||
// eslint-disable-next-line no-console | ||
console.error('Could not resolve dependency entryId: ', dependency.entryId, dependency); | ||
} |
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.
Is the else
case not an error?
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.
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/[email protected](@xxx/[email protected])" will be converted to "//byted/[email protected](@xxx/[email protected])'" -
After being parsed by
parseDependencies
inLockfileDependency
, theentryId
is further processed based on its original content. I suppose the processed results will not appear in thepnpm-lock.yaml
file. For example, content likelink:../../liveart-core/packages/actions
will be transformed byLockfileDependency
to start with 'project', which will not appear in thepnpm-lock.yaml
file. Therefore, I have imposed further restrictions here.
apps/lockfile-explorer/src/start.ts
Outdated
const updatedPackages: Lockfile['packages'] = {}; | ||
for (const dependencyPath in packages) { | ||
if (Object.prototype.hasOwnProperty.call(packages, dependencyPath)) { | ||
updatedPackages[convertLockfileV6DepPathToV5DepPath(dependencyPath)] = packages[dependencyPath]; |
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.
Would it be better to convert V5->V6 instead of vice-versa?
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.
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.
Additionally, I think resolving dependencies in the browser is not very appropriate, as it limits our use of pnpm-related open-source libraries.
apps/lockfile-explorer/src/start.ts
Outdated
const doc = yaml.load(pnpmLockfileText); | ||
const doc = yaml.load(pnpmLockfileText) as Lockfile; | ||
const { packages, lockfileVersion } = doc; | ||
if (packages && lockfileVersion.toString().startsWith('6.')) { |
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.
Is this fix really about supporting multiple versions of the PNPM lockfile format? That might be a better description for the change log.
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.
@@ -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; |
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.
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.
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.
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 <[email protected]>
apps/lockfile-explorer/src/start.ts
Outdated
if (typeof lockfileVersion === 'string') { | ||
shrinkwrapFileMajorVersion = parseInt(lockfileVersion.substring(0, lockfileVersion.indexOf('.')), 10); | ||
} else if (typeof lockfileVersion === 'number') { | ||
shrinkwrapFileMajorVersion = lockfileVersion; |
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.
If it is encoded as a number
, then shrinkwrapFileMajorVersion
should probably be Math.floor(lockfileVersion)
right?
apps/lockfile-explorer/src/start.ts
Outdated
|
||
let shrinkwrapFileMajorVersion: number; | ||
if (typeof lockfileVersion === 'string') { | ||
shrinkwrapFileMajorVersion = parseInt(lockfileVersion.substring(0, lockfileVersion.indexOf('.')), 10); |
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.
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.
@L-Qun Thanks for making these fixes! 🎉 |
Summary
Fix some bugs for the lockfile-explorer tool
How it was tested
Manually tested with Rushstack repo locally.