feat: bundle uuid into stack package and expose schema builder#451
Conversation
🦋 Changeset detectedLatest commit: 9f0b9cc The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis PR exposes ChangesStack Release: columnBuilders API & CJS uuid Fix
Sequence Diagram(s)Possibly related PRs
Suggested reviewers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
| "@cipherstash/stack": minor | ||
| --- | ||
|
|
||
| Fix CJS consumers crashing with `Must use import to load ES Module: .../uuid/dist-node/index.js`. The `uuid` package is pure ESM and has no CJS entry point, so the CJS build of `@cipherstash/stack` could not `require()` it at runtime. `uuid` is now bundled into the CJS output (the ESM build is unchanged). |
There was a problem hiding this comment.
Can we add something to tests/CI to validate this and prevent regressions?
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
I'd prefer to add some test or verification before approving.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stack/__tests__/cjs-require.test.ts`:
- Around line 73-78: The top-level build run using execFileSync guarded by
distIsFresh() should be moved into a test lifecycle hook: wrap the distIsFresh()
check and execFileSync('pnpm', ['run', 'build'], { cwd: packageRoot, stdio:
'inherit' }) inside a beforeAll() in the test suite so builds only run during
test setup (and avoid running at module-load time), and remove the top-level
const cjsEntries = findCjsEntries() so findCjsEntries() is invoked after the
build inside the beforeAll or in tests as needed; reference distIsFresh(),
execFileSync, packageRoot, findCjsEntries(), cjsEntries, and beforeAll() when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d76713fb-597e-4bd7-a051-3a2f0c9cdb87
📒 Files selected for processing (2)
packages/stack/__tests__/cjs-require.test.tspackages/stack/__tests__/schema-builders.test.ts
| if (!distIsFresh()) { | ||
| execFileSync('pnpm', ['run', 'build'], { | ||
| cwd: packageRoot, | ||
| stdio: 'inherit', | ||
| }) | ||
| } |
There was a problem hiding this comment.
Move build execution into a beforeAll() hook.
Running execFileSync at module-load time (outside any test lifecycle hook) can cause race conditions when tests run in parallel and triggers even when executing unrelated test files in the workspace.
🔧 Proposed fix to move build into test lifecycle
Move the freshness check and build inside the test suite:
-if (!distIsFresh()) {
- execFileSync('pnpm', ['run', 'build'], {
- cwd: packageRoot,
- stdio: 'inherit',
- })
-}
-
-const cjsEntries = findCjsEntries()
-
describe('CJS consumers can require the built bundles', () => {
+ let cjsEntries: string[]
+
+ beforeAll(() => {
+ if (!distIsFresh()) {
+ execFileSync('pnpm', ['run', 'build'], {
+ cwd: packageRoot,
+ stdio: 'inherit',
+ })
+ }
+ cjsEntries = findCjsEntries()
+ })
+
it('discovers at least the public entry points', () => {Then remove the const cjsEntries = findCjsEntries() line from the top level (line 80).
📝 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.
| if (!distIsFresh()) { | |
| execFileSync('pnpm', ['run', 'build'], { | |
| cwd: packageRoot, | |
| stdio: 'inherit', | |
| }) | |
| } | |
| describe('CJS consumers can require the built bundles', () => { | |
| let cjsEntries: string[] | |
| beforeAll(() => { | |
| if (!distIsFresh()) { | |
| execFileSync('pnpm', ['run', 'build'], { | |
| cwd: packageRoot, | |
| stdio: 'inherit', | |
| }) | |
| } | |
| cjsEntries = findCjsEntries() | |
| }) | |
| it('discovers at least the public entry points', () => { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/stack/__tests__/cjs-require.test.ts` around lines 73 - 78, The
top-level build run using execFileSync guarded by distIsFresh() should be moved
into a test lifecycle hook: wrap the distIsFresh() check and
execFileSync('pnpm', ['run', 'build'], { cwd: packageRoot, stdio: 'inherit' })
inside a beforeAll() in the test suite so builds only run during test setup (and
avoid running at module-load time), and remove the top-level const cjsEntries =
findCjsEntries() so findCjsEntries() is invoked after the build inside the
beforeAll or in tests as needed; reference distIsFresh(), execFileSync,
packageRoot, findCjsEntries(), cjsEntries, and beforeAll() when making the
change.
3 participants
Summary by CodeRabbit
Bug Fixes
New Features
Tests