feat(tools-scripts): Add tools-scripts package for common cli helpers and routines #3610
Conversation
…lding and provides a api based lint implementation
github-actions
bot
added
the
chore
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * @returns a script context object to use for run commands | ||
| */ | ||
| context(overrides: Partial<ScriptContext> = {}): ScriptContext { | ||
| return { ...RnxCli.defaultContext, ...this.scriptDefaults, ...overrides }; |
There was a problem hiding this comment.
Why do we need to copy this.scriptDefaults here? It should already be a part of RnxCli.defaultContext. We should be able to remove this.scriptDefaults entirely.
| export { BaseCommand } from "./commands.ts"; | ||
| export { createLintCommand } from "./lint-command.ts"; | ||
| export { | ||
| defaultLintConfig as defaultLintOptions, |
There was a problem hiding this comment.
Can we rename defaultLintConfig to defaultLintOptions?
| cwd() { | ||
| return this.context.cwd; | ||
| } | ||
|
|
||
| /** | ||
| * @returns {string} | ||
| */ | ||
| root() { | ||
| return this.context.root; | ||
| } |
There was a problem hiding this comment.
I don't see these being used anywhere. The lint command accesses cwd directly via this.context.cwd. Do we really need these props here?
| type LintConfiguration, | ||
| type LintOptions, | ||
| } from "./lint.ts"; | ||
| import { type ScriptContext } from "./types.ts"; |
There was a problem hiding this comment.
types.ts shouldn't contain anything but types:
| import { type ScriptContext } from "./types.ts"; | |
| import type { ScriptContext } from "./types.ts"; |
| }, | ||
| "peerDependencies": { | ||
| "eslint": "^9.0.0", | ||
| "eslint-formatter-pretty": "^6.0.1" |
There was a problem hiding this comment.
Linting is not formatting. We should keep them separate. ESLint and Prettier will conflict with each other.
| */ | ||
| constructor(root: string, options: Partial<CliOptions> = {}) { | ||
| super(options); | ||
| this.scriptDefaults = { root, cwd: process.cwd() }; |
There was a problem hiding this comment.
Is process.cwd() always the package root? Can these commands be run outside a package manager?
| /** | ||
| * Fix the found issues where possible | ||
| */ |
There was a problem hiding this comment.
Nit: These comments are unnecessary because we also have descriptions.
|
|
||
| // dynamically import eslint and the pretty formatter since these are optional peer dependencies | ||
| const { ESLint } = await import("eslint"); |
There was a problem hiding this comment.
We should catch this and output an actionable error message e.g.:
function importESLint() {
try {
const { ESLint } = import("eslint");
return ESLint;
} catch (_) {
process.exitCode = 1;
}
}
...
// dynamically import eslint and the pretty formatter since these are optional peer dependencies
const ESLint = importESLint();
if (!ESLint) {
console.error("Could not find `eslint`; did you forget to add it as a dev dependency?");
return;
}| const { fallbackConfig, cwd } = options; | ||
|
|
||
| // default to the local config in cwd if it exists | ||
| const eslintConfigPath = path.join(cwd, "eslint.config.js"); |
There was a problem hiding this comment.
ESLint supports many more types today: https://eslint.org/docs/latest/use/configure/configuration-files#configuration-file
| const { cwd, filePatterns = [], globalIgnores = [] } = options; | ||
|
|
||
| // use git to list the files in the directory | ||
| const { stdout } = spawnSync("git", ["ls-files", ...filePatterns], { |
There was a problem hiding this comment.
git ls-files can also exclude files with :!:<pattern>:
| const { stdout } = spawnSync("git", ["ls-files", ...filePatterns], { | |
| const { stdout } = spawnSync("git", ["ls-files", ...filePatterns, globalIgnores.map((p) => `:!:${p}`)], { |
| /** | ||
| * either a list of files to lint, or a list of patterns to use for file matching | ||
| */ | ||
| filesOrPatterns = Option.Rest(); |
There was a problem hiding this comment.
Another thing I've just recalled is that this pattern basically blackboxes the command i.e., you can no longer pass flags directly to ESLint. Sometimes, this is desirable because we cannot keep adding an entry for every new flag that gets added and it prevents people from learning a generic tool like ESLint. In 1JS, we started out like this and it was a pretty bad experience until we opened up the commands. We should not make the same mistake here.
Pull request was converted to draft
Description
Adds a tools-scripts package for common routines and helpers for building CLIs. This will be used to share code amongst our CLI implementations and provide helpers for implementing repo scripts folders.