Use tsdown bundler instead of tsup #109
Conversation
✅ Deploy Preview for detsys-ts-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Caution Review failedThe pull request is closed. WalkthroughOutputs now take the full inputs block and generate per-system artifacts (schemas, formatter, devShells) from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Eval as Flake outputs evaluation
participant Inputs as Inputs (includes nixpkgs, flake-schemas)
participant ForEach as forEachSupportedSystem
participant Nixpkgs as inputs.nixpkgs
participant FlakeSchemas as inputs.flake-schemas
participant PerSystem as Per-system attributes
Note over Eval,Inputs: outputs now receive full inputs block
Eval->>Inputs: pass inputs into outputs ({ self, ... }@inputs)
Eval->>ForEach: invoke forEachSupportedSystem with f
ForEach->>Nixpkgs: import per-system pkgs via inputs.nixpkgs
ForEach->>FlakeSchemas: reference schemas via inputs.flake-schemas
ForEach->>PerSystem: produce per-system artifacts (schemas, formatter, devShells)
PerSystem-->>Eval: return generated per-system attributes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (3)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/index.ts (4)
8-14: Use type-only imports to avoid runtime import errorsThese are type-only. In ESM with verbatim module syntax, importing them as values can throw “does not provide an export” at runtime.
Apply:
-import { CheckIn, Feature } from "./check-in.js"; +import type { CheckIn, Feature } from "./check-in.js"; @@ -import { SourceDef, constructSourceParameters } from "./sourcedef.js"; +import type { SourceDef } from "./sourcedef.js"; +import { constructSourceParameters } from "./sourcedef.js";
17-19: Split UUID into a type importUUID is a type-only export from node:crypto; importing it as a value can break at runtime.
Apply:
-import { Got, Request, TimeoutError } from "got"; +import type { Got, Request } from "got"; +import { TimeoutError } from "got"; @@ -import { UUID, randomUUID } from "node:crypto"; +import type { UUID } from "node:crypto"; +import { randomUUID } from "node:crypto";
21-26: Fix fs constants sourcefs.constants come from node:fs, not node:fs/promises. Current usage risks undefined constants.
Apply:
-import { - PathLike, - WriteStream, - createWriteStream, - readFileSync, -} from "node:fs"; +import { + PathLike, + WriteStream, + createWriteStream, + readFileSync, + constants as fsConstants, +} from "node:fs"; @@ - await chmod(binaryPath, fs.constants.S_IXUSR | fs.constants.S_IXGRP); + await chmod(binaryPath, fsConstants.S_IXUSR | fsConstants.S_IXGRP);Also applies to: 488-490
1189-1192: Wrong fallback for binaryNamesDenyListYou’re defaulting to actionOptions.binaryNamePrefixes instead of binaryNamesDenyList.
Apply:
- binaryNamesDenyList: - actionOptions.binaryNamePrefixes ?? PROGRAM_NAME_CRASH_DENY_LIST, + binaryNamesDenyList: + actionOptions.binaryNamesDenyList ?? PROGRAM_NAME_CRASH_DENY_LIST,
🧹 Nitpick comments (2)
package.json (1)
9-9: Add Node engine baseline and exports map
Suggest in package.json adding:"scripts": { "build": "tsdown", }, "homepage": "https://github.com/DeterminateSystems/detsys-ts#readme", + "engines": { + "node": ">=24" + }, + "exports": { + ".": { + "types": "./dist/index.d.ts", + "default": "./dist/index.js" + } + },tsdown.config.ts (1)
1-13: Config LGTM; adjust Node.js baseline for CI
- import from tsdown and ESM-only output look good.
- GitHub-hosted Actions runners default to Node 22.x (JS runtime baseline: Node 20) as of Sept 8, 2025 cite12.
- To target Node 24, add
actions/setup-node@v3to your workflows to install Node 24 at runtime, or change thetargetin tsdown.config.ts tonode20cite3.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
dist/chunk-Cl8Af3a2.jsis excluded by!**/dist/**dist/index.d.tsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapflake.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
flake.nix(1 hunks)package.json(2 hunks)src/index.ts(1 hunks)tsdown.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run all checks
🔇 Additional comments (6)
package.json (1)
47-47: tsdown@^0.14.2 is the latest and TS 5.9-compatible
npm shows 0.14.2 as the current stable release and tsdown doesn’t impose a strict TypeScript peerDependency, so it works with TypeScript 5.9—just run your build/tests to confirm.src/index.ts (1)
1201-1213: Type-only re-exports: LGTMThe move to export type { … } aligns with ESM and avoids accidental runtime imports.
flake.nix (4)
5-7: Inputs via flakehub: OKURLs look fine and pinned at a channel-like path.
9-29: Outputs shape and per-system plumbing: LGTMAdopting { … }@inputs and genAttrs wiring is clear and conventional.
31-31: Schema re-export: LGTMinherit (inputs.flake-schemas) schemas; is concise and correct.
33-46: Align devShell Node version with build target
- Replace
nodejs_latestwithnodejs_24in yourdevShellspackages to match the Node.js 24 build target.- Confirm that the
nodejs_24attribute (or the correct Node.js 24 attribute name) exists in your pinned nixpkgs; adjust if needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
package.json (1)
9-9: Add explicit ESM exports and Node engine requirement
tsdown emits todist/index.js&dist/index.d.tsby default and supportsdts: { resolve: false }, so your existing"main"/"types"remain valid. In package.json add:{ "exports": { ".": { "types": "./dist/index.d.ts", "default": "./dist/index.js" } }, "engines": { "node": ">=20" } }tsconfig.json (1)
4-5: NodeNext is appropriate; add verbatimModuleSyntax to avoid unintended runtime imports.This helps ensure type-only imports/exports are preserved and not transformed into value imports at runtime.
"compilerOptions": { "target": "es2020", "module": "NodeNext", "moduleResolution": "NodeNext", + "verbatimModuleSyntax": true, "outDir": "./dist",src/index.ts (1)
1201-1215: Useimport typefor type-only symbols
Avoid unnecessary runtime imports of types from./check-in.jsand./sourcedef.js.- import { CheckIn, Feature } from "./check-in.js"; + import type { CheckIn, Feature } from "./check-in.js"; - import { SourceDef, constructSourceParameters } from "./sourcedef.js"; + import type { SourceDef } from "./sourcedef.js"; + import { constructSourceParameters } from "./sourcedef.js";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
dist/chunk-Cl8Af3a2.jsis excluded by!**/dist/**dist/index.d.tsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapflake.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
flake.nix(1 hunks)package.json(2 hunks)src/index.ts(1 hunks)tsconfig.json(1 hunks)tsdown.config.ts(1 hunks)
🔇 Additional comments (1)
tsdown.config.ts (1)
1-1: LGTM: defineConfig import switched to tsdown.Config structure matches tsdown usage.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/index.ts (2)
1018-1026: Use fsConstants with fs.promises.accessfs.promises does not expose constants; this will fail at runtime.
- await fs.access(candidateNix, fs.constants.X_OK); + await fs.access(candidateNix, fsConstants.X_OK);
1193-1195: Wrong option used for deny list defaultYou’re defaulting binaryNamesDenyList from binaryNamePrefixes. Use the correct field.
- binaryNamesDenyList: - actionOptions.binaryNamePrefixes ?? PROGRAM_NAME_CRASH_DENY_LIST, + binaryNamesDenyList: + actionOptions.binaryNamesDenyList ?? PROGRAM_NAME_CRASH_DENY_LIST,
♻️ Duplicate comments (3)
tsdown.config.ts (1)
7-7: Targeting node20 is correct for GitHub Actions runnersThis resolves earlier concern about node24 vs node20.
flake.nix (2)
5-6: FlakeHub URLs should include .tar.gzUse the documented tarball form for versioned inputs.
- flake-schemas.url = "https://flakehub.com/f/DeterminateSystems/flake-schemas/0.1"; - nixpkgs.url = "https://flakehub.com/f/NixOS/nixpkgs/0"; + flake-schemas.url = "https://flakehub.com/f/DeterminateSystems/flake-schemas/0.1.tar.gz"; + nixpkgs.url = "https://flakehub.com/f/NixOS/nixpkgs/0.tar.gz";
33-34: Use pkgs.nixfmt instead of deprecated nixfmt-rfc-styleRecent nixpkgs promotes nixfmt; swap formatter.
- formatter = forEachSupportedSystem ({ pkgs }: pkgs.nixfmt-rfc-style); + formatter = forEachSupportedSystem ({ pkgs }: pkgs.nixfmt);
🧹 Nitpick comments (1)
src/index.ts (1)
23-28: Prettier/import order nit: fix named import orderingStatic analysis flagged ordering of constants vs readFileSync.
-import { - PathLike, - WriteStream, - createWriteStream, - readFileSync, - constants as fsConstants, -} from "node:fs"; +import { + PathLike, + WriteStream, + createWriteStream, + constants as fsConstants, + readFileSync, +} from "node:fs";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
dist/chunk-Cl8Af3a2.jsis excluded by!**/dist/**dist/index.d.tsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapflake.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
flake.nix(1 hunks)package.json(2 hunks)src/index.ts(3 hunks)tsconfig.json(1 hunks)tsdown.config.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/index.ts
[error] 26-27: Replace readFileSync,⏎··constants·as·fsConstants with constants·as·fsConstants,⏎··readFileSync
(prettier/prettier)
🔇 Additional comments (3)
tsconfig.json (1)
4-4: Module set to NodeNext aligns with ESM + tsdownMatches package.json "type": "module" and ESM output. No issues.
package.json (2)
47-47: tsdown devDependency addedVersion looks reasonable; no concerns here.
9-9: Switched build to tsdown—verify output path matches main/typesEnsure tsdown emits to dist (index.js and index.d.ts) to satisfy "main" and "types".
You can confirm locally: run build and check artifacts exist:
#!/bin/bash pnpm run build test -f dist/index.js && test -f dist/index.d.ts && echo "OK: dist outputs present" || { echo "Missing dist outputs"; exit 1; }
| async fetchExecutable(): Promise<string> { | ||
| const binaryPath = await this.fetchArtifact(); | ||
| await chmod(binaryPath, fs.constants.S_IXUSR | fs.constants.S_IXGRP); | ||
| await chmod(binaryPath, fsConstants.S_IXUSR | fsConstants.S_IXGRP); | ||
| return binaryPath; | ||
| } |
There was a problem hiding this comment.
chmod clears read bits; preserve existing mode and add +x
Passing only exec flags replaces the whole mode, potentially yielding unreadable binaries. OR the exec bits onto current mode (and include others exec if desired).
async fetchExecutable(): Promise<string> {
const binaryPath = await this.fetchArtifact();
- await chmod(binaryPath, fsConstants.S_IXUSR | fsConstants.S_IXGRP);
+ const st = await fs.stat(binaryPath);
+ const newMode =
+ st.mode |
+ fsConstants.S_IXUSR |
+ fsConstants.S_IXGRP |
+ fsConstants.S_IXOTH;
+ await chmod(binaryPath, newMode);
return binaryPath;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fetchExecutable(): Promise<string> { | |
| const binaryPath = await this.fetchArtifact(); | |
| await chmod(binaryPath, fs.constants.S_IXUSR | fs.constants.S_IXGRP); | |
| await chmod(binaryPath, fsConstants.S_IXUSR | fsConstants.S_IXGRP); | |
| return binaryPath; | |
| } | |
| async fetchExecutable(): Promise<string> { | |
| const binaryPath = await this.fetchArtifact(); | |
| const st = await fs.stat(binaryPath); | |
| const newMode = | |
| st.mode | | |
| fsConstants.S_IXUSR | | |
| fsConstants.S_IXGRP | | |
| fsConstants.S_IXOTH; | |
| await chmod(binaryPath, newMode); | |
| return binaryPath; | |
| } |
🤖 Prompt for AI Agents
In src/index.ts around lines 489-493 the code calls chmod with only exec flags
which overwrites the file mode and can clear read bits; change it to first read
the current mode (fs.stat or fs.promises.stat), OR in the execute bits
(fsConstants.S_IXUSR | fsConstants.S_IXGRP | fsConstants.S_IXOTH) with the
existing mode, then call chmod with that combined mode so existing read/write
bits are preserved (and include S_IXOTH if you want others to be executable).
cole-h
left a comment
cole-h
left a comment
There was a problem hiding this comment.
Seems OK to me, but CI is failing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
flake.nix (2)
5-6: Fix FlakeHub input URL form (.tar.gz suffix).
Prior review already flagged this; recommend switching to tarball URLs.- flake-schemas.url = "https://flakehub.com/f/DeterminateSystems/flake-schemas/0.1"; - nixpkgs.url = "https://flakehub.com/f/NixOS/nixpkgs/0"; + flake-schemas.url = "https://flakehub.com/f/DeterminateSystems/flake-schemas/0.1.tar.gz"; + nixpkgs.url = "https://flakehub.com/f/NixOS/nixpkgs/0.tar.gz";
34-34: Use pkgs.nixfmt (fallback to -rfc-style for older nixpkgs).
nixfmt-rfc-styleis being phased out; preferpkgs.nixfmtwith a compat fallback.- formatter = forEachSupportedSystem ({ pkgs, ... }: pkgs.nixfmt-rfc-style); + formatter = forEachSupportedSystem ({ pkgs, ... }: + if pkgs ? nixfmt then pkgs.nixfmt else pkgs.nixfmt-rfc-style + );
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
18-20: Make nixfmt check robust and align with flake formatter.
Use NUL-delimited paths and avoid running when no .nix files; or leveragenix fmtwhich uses your flakeformatter.Apply one of the following:
- - name: Check Nix formatting - run: git ls-files '*.nix' | xargs nix develop --command nixfmt --check + - name: Check Nix formatting + run: git ls-files -z '*.nix' | xargs -0 -r nix develop --command nixfmt --checkOr:
- - name: Check Nix formatting - run: git ls-files '*.nix' | xargs nix develop --command nixfmt --check + - name: Check Nix formatting + run: nix develop --command nix fmt -- --check
33-38: Pre-docs cleanliness checks look good.
Consider also asserting cleanliness after docs build (see below).
39-44: Add a post-docs cleanliness check.
If docs generation mutates tracked files, catch it explicitly.- name: Build docs run: | rm -rf dist nix develop --command pnpm run docs + + - name: Ensure no staged changes after docs + run: git diff --exit-code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)flake.nix(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run all checks
🔇 Additional comments (10)
.github/workflows/ci.yml (5)
12-12: Runner bump to ubuntu-24.04 looks good.
No issues spotted with the update.
17-17: No action for extra blank line.
Harmless; leaving as-is.
24-26: Lint step OK.
Uses nix shell; no concerns.
27-29: Test step OK.
No issues.
30-32: Build step OK.
Consistent with the tsdown switch.flake.nix (5)
9-13: outputs args shape LGTM.
Pattern{ self, ... }@inputs:is correct and future-proof.
15-20: Supported systems list LGTM.
Matches common x86_64/aarch64 Linux/Darwin set.
21-29: forEachSupportedSystem helper is clean.
Good use ofinputs.nixpkgs.lib.genAttrsand per-system import.
32-32: Publicly re-exporting schemas is fine.
Clear and minimal surface.
36-47: Dev shell contents LGTM; minor nicety: rely on formatter attr directly.
Usingself.formatter.${system}is fine; keeping the formatter in shell is convenient. No blockers.
lucperkins
temporarily deployed
to
detsys-pr-bot
GitHub Actions
Inactive
lucperkins
temporarily deployed
to
detsys-pr-bot
GitHub Actions
Inactive
lucperkins
temporarily deployed
to
detsys-pr-bot
GitHub Actions
Inactive
lucperkins
temporarily deployed
to
detsys-pr-bot
GitHub Actions
Inactive
lucperkins
temporarily deployed
to
detsys-pr-bot
GitHub Actions
Inactive
lucperkins
temporarily deployed
to
detsys-pr-bot
GitHub Actions
Inactive
tsup is no longer actively maintained, so we should switch to the latest and greatest (at least according to the tsup project itself).
Summary by CodeRabbit
New Features
Refactor
Chores
Breaking Changes