feat: modernize circuits build pipeline #876
Conversation
|
Warning Rate limit exceeded@transphorm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
WalkthroughSwitches circuits package to a tsup-based dual ESM/CJS build with emitted types, adds package exports/main/module/types/files, introduces post-build shim generation script and shim config placeholder, and adds a barrel src entry plus tsconfig files for both outputs. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Yarn as yarn/npm
participant TSUP as tsup
participant TSC as tsc (decl only)
participant PB as postBuild.mjs
participant FS as File System
Dev->>Yarn: yarn build
Yarn->>TSUP: tsup (build ESM -> dist/esm)
TSUP->>FS: write dist/esm/*
Yarn->>TSUP: tsup (build CJS -> dist/cjs)
TSUP->>FS: write dist/cjs/*
Yarn->>TSC: tsc --emitDeclarationOnly -> dist/src/index.d.ts
TSC->>FS: write declarations
Yarn->>Yarn: mv dist/src/index.d.ts -> dist/esm/index.d.ts
Yarn->>PB: node ./scripts/postBuild.mjs
PB->>FS: write dist/package.json, dist/esm/package.json, dist/cjs/package.json
PB->>FS: create shim files per shimConfigs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
transphorm
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
circuits/scripts/shimConfigs.js (1)
1-2: Document and freeze shim config shapeMinor: add a typedef for clarity and freeze the array to avoid accidental mutation.
-// Shim configurations for @selfxyz/circuits -export const shimConfigs = []; +// Shim configurations for @selfxyz/circuits +/** + * @typedef {{ shimPath: string; targetPath: string }} ShimConfig + * @type {ReadonlyArray<ShimConfig>} + */ +export const shimConfigs = Object.freeze([]);circuits/tsconfig.cjs.json (1)
4-9: Align moduleResolution with NodeNext for consistencyUsing moduleResolution: "Node" while the base config uses "NodeNext" can cause subtle path/extension resolution differences. Consider using NodeNext here too to match Node 22 behavior.
- "moduleResolution": "Node", + "moduleResolution": "NodeNext",circuits/tsup.config.ts (2)
1-7: Use node: specifier and set platform explicitlyMinor ergonomics: import built-ins via node: scheme, and set platform: 'node' explicitly.
-import path from 'path'; -import { fileURLToPath } from 'url'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url';
12-35: Tweak build flags for clarity and slimmer bundles
- Explicitly set platform: 'node' (tsup default, but being explicit helps).
- Enable treeshake for better dead-code elimination.
- Target modern Node for optimal output.
export default defineConfig([ { tsconfig: './tsconfig.json', entry, format: ['esm'], outDir: path.resolve(__dirname, 'dist/esm'), dts: false, splitting: false, clean: true, sourcemap: true, - target: 'es2020', + target: 'node22', + platform: 'node', + treeshake: true, }, { tsconfig: './tsconfig.cjs.json', entry, format: ['cjs'], outDir: path.resolve(__dirname, 'dist/cjs'), dts: false, splitting: false, clean: false, sourcemap: true, - target: 'es2020', + target: 'node22', + platform: 'node', + treeshake: true, }, ]);circuits/package.json (3)
19-22: Publish surface is good; consider adding sideEffects for better tree-shakingIf modules are side-effect free at top level, set "sideEffects": false to help bundlers.
"files": [ "dist/**/*", "circuits/**/*" ], + "sideEffects": false,
23-25: Build pipeline order is sound; small safety improvementConsider guarding postbuild to fail loudly if dist artifacts are missing (e.g., tsup failed). Alternatively, make postbuild idempotent and verify dist exists before writing.
8-15: Exports are correct; no deep imports detectedI ran a repo-wide search for
@selfxyz/circuits/…in all JS/TS files and found no deep imports, so your current wildcard subpath ("./circuits/*": "./circuits/*") covers existing usage. You can defer adding explicit subpath mappings until you actually expose new internals.Recommendations:
- Verify that the package is truly side-effect-free (no global state mutations, polyfills, etc.)
- If it is, add
to circuits/package.json to unlock better tree-shaking."sideEffects": false
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
circuits/package.json(3 hunks)circuits/scripts/postBuild.mjs(1 hunks)circuits/scripts/shimConfigs.js(1 hunks)circuits/src/index.ts(1 hunks)circuits/tsconfig.cjs.json(1 hunks)circuits/tsconfig.json(1 hunks)circuits/tsup.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-01T17:11:39.232Z
Learnt from: transphorm
PR: selfxyz/self#823
File: common/package.json:21-60
Timestamp: 2025-08-01T17:11:39.232Z
Learning: Explicit export mappings in package.json exports field provide better tree shaking than wildcard patterns because they enable easier static analysis by bundlers, allowing for more precise dependency graph understanding and better dead code elimination.
Applied to files:
circuits/src/index.ts
🧬 Code Graph Analysis (2)
circuits/scripts/postBuild.mjs (1)
circuits/scripts/shimConfigs.js (2)
shimConfigs(2-2)shimConfigs(2-2)
circuits/tsup.config.ts (2)
common/scripts/testExports.js (1)
__filename(12-12)circuits/scripts/postBuild.mjs (1)
__dirname(6-6)
🔇 Additional comments (1)
circuits/package.json (1)
36-38: Fix types script: avoid invoking full build pipelinetypes currently runs yarn build --noEmit which passes an unknown flag to tsup and re-invokes postbuild. Use tsc for type-check only.
- "types": "yarn build --noEmit" + "types": "tsc -p tsconfig.json --noEmit"Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
circuits/tsconfig.json (1)
16-16: Exclude tests from the base tsconfig to avoid leaking test code into the library buildIncluding tests//* (and tests//*.json) in the base config can:
- Confuse tooling and type resolution.
- Risk accidental bundling of test-only code/helpers.
- Increase build surface for editors/tsserver.
Limit base tsconfig to src only; keep tests under tsconfig.spec.json.
- "include": ["tests/**/*", "tests/**/*.json", "src/**/*"], + "include": ["src/**/*"],
🧹 Nitpick comments (2)
circuits/package.json (2)
8-15: Add a react-native condition to exports for MetroTo make Metro prefer the ESM build without heuristics, add a react-native condition to the root export.
"exports": { ".": { - "types": "./dist/esm/index.d.ts", - "import": "./dist/esm/index.js", - "require": "./dist/cjs/index.cjs" + "types": "./dist/esm/index.d.ts", + "react-native": "./dist/esm/index.js", + "import": "./dist/esm/index.js", + "require": "./dist/cjs/index.cjs" }, "./circuits/*": "./circuits/*" },
19-22: Double-check publishing size and RN compatibility for './circuits/*' files"files": ["dist//*", "circuits//*"] will publish everything under circuits, which may include large artifacts (e.g., wasm, zkey) and Node-centric assets. If only shims are needed, consider narrowing the pattern to exactly what Metro consumes.
Please confirm the minimum subpaths needed by Metro shims (postBuild). If feasible, prefer something like:
"files": ["dist/**/*", "circuits/**/*.json", "circuits/**/*.shim.*", "circuits/**/*.js"]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
circuits/package.json(2 hunks)circuits/scripts/postBuild.mjs(1 hunks)circuits/src/index.ts(1 hunks)circuits/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- circuits/scripts/postBuild.mjs
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run_circuit_tests
- GitHub Check: build
🔇 Additional comments (1)
circuits/package.json (1)
102-102: LGTM: tsup devDependencyAdopting tsup is appropriate here and aligns with your dual ESM/CJS strategy.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
transphorm
deleted the
codex/add-circuits-build-configuration-and-scripts
branch
Summary
Testing
yarn workspace @selfxyz/circuits niceyarn lintyarn buildyarn typesyarn workspace @selfxyz/circuits test(fails: Cannot read properties of undefined (reading 'rsa'))https://chatgpt.com/codex/tasks/task_b_68993d84b87c832da8aa191136fa5a71
Summary by CodeRabbit
New Features
Chores
Tests