Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Dec 18, 2025

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

Project Deployment Review Updated (UTC)
rivet-inspector Ready Ready Preview, Comment Dec 18, 2025 10:45pm
rivetkit-serverless Error Error Dec 18, 2025 10:45pm
2 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Dec 18, 2025 10:45pm
rivet-site Ignored Ignored Preview Dec 18, 2025 10:45pm

Copy link
Member Author


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

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 Dec 18, 2025

Pull Request Review: fix(rivetkit): use shims for cjs imports

Summary

This PR adds the shims: true option to the tsup base configuration to properly handle import.meta.url in CommonJS builds. This is a focused fix addressing compatibility issues with dual-format package builds.


✅ Code Quality & Best Practices

Positive:

  • Minimal, focused change: The PR makes exactly one logical change - enabling shims in tsup configuration
  • Proper versioning: Correctly bumps to an RC version (2.0.29-rc.1) for testing before release
  • Typo in comment: Line 26 of tsup.base.ts has a typo: import.meta.ur. should be import.meta.url

Suggestion:

-	// Required to replace `import.meta.ur.` with CJS shims
+	// Required to replace `import.meta.url` with CJS shims

🔍 Technical Analysis

Context:
The codebase uses import.meta.url in rivetkit-typescript/packages/rivetkit/src/utils/node.ts:34 within the getRequireFn() function:

return createRequire(import.meta.url);

Why this change is needed:

  • import.meta.url is an ESM-only feature that doesn't exist in CommonJS
  • Without shims, tsup cannot properly transpile this to CJS, causing runtime errors
  • The shims: true option tells tsup to inject polyfills/shims for ESM-specific features like import.meta.url

Impact:

  • Affects all packages using tsup.base.ts (rivetkit packages, engine SDKs, frontend packages)
  • Enables proper dual-format (ESM + CJS) package distribution
  • Critical for Node.js environments that still use CommonJS

🐛 Potential Issues

No issues identified - this is a straightforward configuration fix.


⚡ Performance Considerations

Neutral impact:

  • Shims add minimal overhead (a few bytes of polyfill code)
  • Only affects build output size, not runtime performance
  • The overhead is negligible compared to the benefit of CJS compatibility

🔒 Security Concerns

No security concerns - this is a build configuration change with no security implications.


🧪 Test Coverage

Observations:

  • No tests modified (appropriate for a build configuration change)
  • This change should be validated through:
    1. Building the affected packages
    2. Testing in a CommonJS-only Node.js environment
    3. Verifying import.meta.url usage works in CJS builds

Recommendation:
Before merging, verify that:

# Build the packages
pnpm build

# Test in a CJS environment (e.g., create a test script that requires the CJS build)
node -e "const rivetkit = require('./rivetkit-typescript/packages/rivetkit/dist/index.cjs'); console.log('CJS import successful');"

📋 Additional Notes

  1. Missing files entry: The engine/sdks/typescript/runner/package.json change adds a files array. While this seems unrelated to the shims fix, it's actually a positive improvement for proper package publishing.

  2. Workspace consistency: All package version bumps are consistent (2.0.28 → 2.0.29-rc.1), including Cargo packages.

  3. Documentation: Consider adding a comment in tsup.base.ts explaining when shims should be enabled/disabled to help future maintainers.


✨ Verdict

✅ Approve with minor typo fix

This is a well-scoped PR that addresses a real compatibility issue. The only actionable item is fixing the typo in the comment on line 26 of tsup.base.ts.

Suggested action before merge:

  1. Fix the typo: import.meta.ur.import.meta.url
  2. Verify CJS builds work correctly in a test environment
  3. Merge and release as RC for validation

Review completed on 2025-12-18

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 18, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3658

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3658

commit: 57b9eb4

@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 19, 2025

Merge activity

  • Dec 19, 7:20 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Dec 19, 7:21 AM UTC: CI is running for this pull request on a draft pull request (#3661) due to your merge queue CI optimization settings.
  • Dec 19, 7:22 AM UTC: Merged by the Graphite merge queue via draft PR: #3661.

graphite-app bot pushed a commit that referenced this pull request Dec 19, 2025
@graphite-app graphite-app bot closed this Dec 19, 2025
@graphite-app graphite-app bot deleted the 12-18-fix_rivetkit_use_shims_for_cjs_imports branch December 19, 2025 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants