Conversation
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
commit: |
| "types": "tsc --noEmit", | ||
| "deps": "pnpm exec knip --production", | ||
| "test": "pnpm --color -r run test", | ||
| "deps": "pnpm dlx knip --production", |
There was a problem hiding this comment.
bsh lint already runs knip afaiu. double check but we may be able to remove this script entirely
There was a problem hiding this comment.
There is a workflow that explicitly uses the deps script. It can be removed! Running bsh lint instead at the workflow won't pass due to oxc. A whole host of errors and warnings are being triggered.
There was a problem hiding this comment.
agree it can't be bsh lint yet, it exits 1 here with 59 errors, and its knip findings are warnings that never fail the run, without --production either. but pnpm dlx pulls knip outside the lockfile on every ci run, so the gate's version floats. could we keep knip as pinned devDep and leave the script as pnpm exec kinp --production?
dreyfus92
left a comment
dreyfus92
left a comment
There was a problem hiding this comment.
thanks for picking this up @mixto! i went a bit ham and down the rabbit hole with this one and there are a couple of loose ends we need to figure out, this will remain blocked until we figure out how to address all the stuff we need before making the shift.
the big one is that the test job is green but the suite is failing. the run on this pr reports 1698 failed | 796 passed and still exits 0, because bsh test never passes vitest's exit code thru. we need to fix this in tools, otherwise nothing catches a broken test once it lands.
once the exit code is honest there are real failures. 17 of the 19 prompt suites fail because the serialaizer bsh uses writes <cyan> style tags where our snopshots have raw escapes. bsh test -u fixes all of them and it's notation only.
the root bsh tes also runs everything 3 to 5 times, since the tools config excludes node_modules/** for some reason instead of **/node_modules/** and vitest walks into examples/*/node_modules/@clack/*. even with that excluded path.test.ts fails from rooth because vi.mock('node:fs') looks for __mocks__ relative to the vitest root. maybe we could keep pnpm -r run test like main had? with regenerated snapshots that passes both packages, though it still needs the tools fix to actually fail on failures.
cc. @43081j
| "dev": "pnpm --filter @example/changesets run start", | ||
| "format": "biome check --write", | ||
| "lint": "biome lint --write --unsafe", | ||
| "format": "bsh format", |
There was a problem hiding this comment.
heads up that once this merges the format workflow runs bsh format on main and auto-commits the result. bsh format --check flags 67 files here, including src, tests, the workflow yaml and CONTRIBUTING.md. the ignorePatterns in tools' oxfmt config resolve relative to that config's own folder, so they don't cover our md/yml/json. could we run it in this pr so the reformat gets reviewed?
| }, | ||
| "scripts": { | ||
| "build": "unbuild", | ||
| "build": "bsh build", |
There was a problem hiding this comment.
building prompts writes 38 untracked .d.ts and .d.ts.map files into packages/core/src, and since pretest builds, every pnpm test leaves them behind. the dts step picks up packages/prompts/tsconfig.json as its root, and the root paths alias pulls core's sources in from outside it, so their declarations land next to them. dropping packages/prompts/tsconfig.json so it falls back to the root one builds with no stray files. could we try that here?
| "noUnusedParameters": true, | ||
| "noUnusedLocals": true, | ||
| "noUncheckedIndexedAccess": true, | ||
| "lib": ["ES2022"], |
There was a problem hiding this comment.
the tools config brings lib up to es2024, but our engines is >= 20.12.0. i checked, Promise.withResolvers and Object.groupBy typecheck with this config and neither exists on node 20. could we keep "lib": ["ES2022"] in our override?
3 participants
What does this PR do?
Migration to the organisation’s tooling using
@bomb.sh/tools. This pull request updates the repository in relation to this #506 other one, which is very out of date.I have used the tsconfig.json configuration from the tools repository. I have left the compiler options that were not defined in the tools configuration.
The
depsscript usesknipto check dependencies. Thebsh lintcommand in the tools already usesknipwhen run. As the script is also used in workflows, it should be left as it is. I usepnpm dlxto call theknipbinary directly.