Conversation
|
There is a type error with the wasm bundle, not sure what to do about that one. |
There was a problem hiding this comment.
Code Review
This pull request upgrades TypeScript to version 6.0.2 and updates several dependencies, including ESLint plugins and GitHub Actions libraries. A new clean command is introduced to the build tools to handle the removal of compiled assets. Feedback on the implementation of this command points out that error results are currently ignored and suggests simplifying the file system removal logic by removing redundant directory checks. Additionally, the global disabling of the @typescript-eslint/only-throw-error rule is flagged as a regression in code quality.
.github/actions/src/lockfiles.ts
Outdated
| try { | ||
|
|
||
| const { exitCode } = await getExecOutput( | ||
| 'git --no-pager diff --quiet origin/master -- yarn.lock', | ||
| [], |
There was a problem hiding this comment.
Bug: The getExecOutput function is called with the entire command as a single string, instead of splitting it into the executable name and an array of arguments.
Severity: HIGH
Suggested Fix
Refactor the getExecOutput call to separate the executable from its arguments. The first argument should be the string 'git', and the second argument should be an array of strings containing the command's flags and parameters: ['--no-pager', 'diff', '--quiet', 'origin/master', '--', 'yarn.lock'].
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/actions/src/lockfiles.ts#L164-L172
Potential issue: In the `hasLockFileChanged` function, the call to `getExecOutput` is
incorrect. The entire git command is passed as a single string to the first argument,
whereas the function expects the executable name (`'git'`) as the first argument and the
command-line arguments as a separate array of strings. This will cause a runtime error
when the system attempts to execute a non-existent file named `"git --no-pager diff
--quiet origin/master -- yarn.lock"`. This error will cause the GitHub Actions workflow
to fail whenever it tries to detect lockfile changes.
.github/actions/src/info/index.ts
Outdated
| throw error; | ||
| } | ||
| const output: Record<string, RawPackageRecord> = {}; | ||
| const { stdout } = await getExecOutput('yarn workspaces list --json', [], { silent: true }); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -177,24 +167,28 @@ export function processRawPackages(topoOrder: string[], packages: Record<string, | |||
| * Retrieve all packages from within the git repository, sorted into the different types | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@RichDom2185 I've figured out what the Yarn error was. The error was occurring with the running of the The error is hidden because the code that runs const { stdout: output, exitCode, stderr } = await getExecOutput('yarn', ['why', pkgName, '--json'], { silent: true });
if (exitCode !== 0) {
core.error(stderr);
throw new Error(`yarn why for ${pkgName} failed!`);
}The The combined outcome is command is essentially failing silently. |
RichDom2185
left a comment
There was a problem hiding this comment.
A number of ESLint plugins (notably React) don't support v10 yet so I'm not surprised things are failing, but I didn't really look into it since should we even look into updating ESLint to v10 when the rest of the ecosystem hasn't caught up?
| files: ['**/*.tsx'], | ||
| plugins: { | ||
| // @ts-expect-error React hooks plugin wrong type | ||
| 'react-hooks': reactHooksPlugin, |
There was a problem hiding this comment.
If we're updating to v7, then we should use reactHooks.configs.flat['recommended'] and then just disable unneeded rules in a subsequent block, no manual plugin registration needed.
There was a problem hiding this comment.
The plugin is no longer a plugin?
There was a problem hiding this comment.
It does seem like the default export still exports the plugin itself (based on their docs); I guess this is fine though with flat config I feel like we should break it up into hierarchies instead of large objects, hence the suggestion.
Migrate everything in the repository to Typescript 6 and update some dependencies.
For some reason the ESLint 10 config algorithm doesn't work, so I've left off on updating ESLint to V10.
Also fix some code that wasn't always behaving properly.