-
-
Notifications
You must be signed in to change notification settings - Fork 252
Fix lint:teams package script #6941
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
Conversation
After the Yarn upgrade, the `lint:teams` package script prints an error indicating that the `npmMinimalAgeGate` configuration option is unrecognized. This script uses the Yarn API to get the list of workspaces, so it's possible something changed internally. This commit fixes the script so that it uses `yarn workspaces` to get information about the workspaces, hopefully avoiding any future breakages.
| return project.workspaces.filter((workspace) => { | ||
| return !workspace.manifest.private; | ||
| }); | ||
| return stdout.split('\n').map((line) => JSON.parse(line)); |
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.
Bug: Trailing Newlines Break JSON Parsing
The getPublicWorkspaces function splits yarn workspaces list stdout by newlines and parses each line as JSON. Command-line tools often output trailing newlines or empty lines, which causes JSON.parse('') to throw a SyntaxError and crash the script. This is a regression in error handling.
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 suppose so. We use this in other scripts though and it hasn't been a problem there, so I'm okay with this.
| @@ -1,9 +1,13 @@ | |||
| import { readJsonFile } from '@metamask/utils/node'; | |||
| import { getPluginConfiguration } from '@yarnpkg/cli'; | |||
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 you remove these dependencies? This was the only place they were used
scripts/lint-teams-json.ts
Outdated
| }; | ||
|
|
||
| // Run the script immediately. | ||
| main().catch(console.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.
We should reconsider this .catch handler. It ended up silencing the 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.
Oh... maybe we need to exit with a non-zero exit code here, would that work?
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.
Yep, exactly. See the create-package script for example, where we do this:
cli(process.argv, commands).catch((error) => {
console.error(error);
process.exitCode = 1;
});
| // The package names in teams.json omit the leading "@", so we do that here | ||
| // too in order to be consistent | ||
| return structUtils.stringifyIdent(packageName).slice(1); | ||
| return workspace.name.slice(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.
Bug: Unchecked Assumptions: Silent Data Corruption
The code assumes all package names start with "@" and blindly calls .slice(1) without validation. If a workspace has a package name that doesn't start with "@", the first character will be incorrectly removed, causing silent data corruption. The old code had explicit null checking and error handling that was removed.
Gudahtt
left a comment
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!
| return project.workspaces.filter((workspace) => { | ||
| return !workspace.manifest.private; | ||
| }); | ||
| return stdout.split('\n').map((line) => JSON.parse(line)); |
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.
Bug: Robust JSON Parsing for Command Output
The stdout.split('\n').map((line) => JSON.parse(line)) pattern will fail if the command output includes a trailing newline, which would create an empty string that causes JSON.parse to throw a SyntaxError. The code should filter out empty lines before parsing, such as using stdout.split('\n').filter(line => line.trim()).map((line) => JSON.parse(line)).
Explanation
With the Yarn upgrade, the
lint:teamspackage script now prints an error indicating that thenpmMinimalAgeGateconfiguration option is unrecognized. This script uses the Yarn API to get the list of workspaces, so it's possible something changed internally to where we now need a different incantation to initialize Yarn.This commit fixes the script so that it uses
yarn workspacesto get information about the workspaces instead of the Yarn API. This has always been known to work in the past so hopefully it should avoid any future breakages.References
(N/A)
Checklist
Note
Replace Yarn API usage with
yarn workspaces listinscripts/lint-teams-json.ts, removing related@yarnpkg/*dev deps.scripts/lint-teams-json.ts:yarn workspaces list --json --no-privateviaexeca.Workspacetype; parse JSON lines; adjust package name derivation.main()invocation with error handling and nonzero exit on failure.@yarnpkg/cli,@yarnpkg/core,@yarnpkg/fslib) frompackage.json.Written by Cursor Bugbot for commit d2f9cc0. This will update automatically on new commits. Configure here.