Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Sep 18, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Sep 18, 2025 6:10am

Copy link
Member Author

NathanFlurry commented Sep 18, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Sep 18, 2025

Claude finished @NathanFlurry's task —— View job


PR Review: sideEffects Configuration

Todo List:

  • Read all modified package.json files to understand the changes
  • Analyze sideEffects configuration for correctness and consistency
  • Check impact on bundling and tree-shaking
  • Evaluate code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Review security implications
  • Provide comprehensive feedback

Summary

This PR changes the sideEffects field from false to specific chunk file patterns across 8 RivetKit packages. The change allows tree-shaking to properly handle tsup-generated chunk files that may contain side effects.

✅ Code Quality & Best Practices

Excellent consistency: All packages follow the same pattern:

"sideEffects": [
  "./dist/chunk-*.js",
  "./dist/chunk-*.cjs"
]

The rivetkit package correctly uses its specific output directory:

"sideEffects": [
  "./dist/tsup/chunk-*.js", 
  "./dist/tsup/chunk-*.cjs"
]

Alignment with build setup: The change aligns perfectly with the tsup configuration that has splitting: true enabled, which generates these chunk files for ESM compatibility.

✅ Technical Correctness

Proper understanding of sideEffects:

  • Previously "sideEffects": false told bundlers that ALL files in these packages were side-effect free
  • The new configuration correctly identifies that chunk files may contain side effects while preserving tree-shaking for other files
  • This is the right approach for packages using code splitting

Accurate file patterns: The glob patterns ./dist/chunk-*.js and ./dist/chunk-*.cjs correctly target tsup's naming convention for split chunks.

✅ Performance Considerations

Positive impact: This change will likely improve bundling performance and correctness:

  • Bundlers can still tree-shake non-chunk files aggressively
  • Chunk files are correctly preserved, preventing potential runtime errors
  • More precise than marking entire packages as having side effects

✅ Security & Safety

No security concerns: This is a build configuration change that doesn't introduce any security vulnerabilities.

❓ Test Coverage

Missing context: The PR lacks:

  • Description of what prompted this change
  • Test plan or verification that the change resolves specific bundling issues
  • Whether this was driven by user reports or preventive maintenance

📋 Recommendations

  1. Add PR description: Consider adding context about what bundling issues this resolves
  2. Verify chunk generation: Ensure that all packages actually generate chunk files when built
  3. Documentation: Consider documenting this pattern in the project's build guidelines

Overall Assessment

This is a well-executed, technically sound change that properly configures sideEffects for packages using code splitting. The consistency across all packages and alignment with the tsup configuration demonstrates good understanding of the bundling ecosystem.

Approval recommended


@pkg-pr-new
Copy link

pkg-pr-new bot commented Sep 18, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/actor@1283

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/cloudflare-workers@1283

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/core@1283

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/db@1283

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/framework-base@1283

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/next-js@1283

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/react@1283

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivetkit@1283

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/sql-loader@1283

commit: d14e754

@graphite-app
Copy link

graphite-app bot commented Sep 24, 2025

Merge activity

  • Sep 24, 6:06 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Sep 24, 6:07 AM UTC: CI is running for this pull request on a draft pull request (#1294) due to your merge queue CI optimization settings.
  • Sep 24, 6:08 AM UTC: Merged by the Graphite merge queue via draft PR: #1294.

graphite-app bot pushed a commit that referenced this pull request Sep 24, 2025
@graphite-app graphite-app bot closed this Sep 24, 2025
@graphite-app graphite-app bot deleted the 09-17-chore_list_sideeffects branch September 24, 2025 06:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant