Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Nov 4, 2025

Summary by CodeRabbit

  • Chores

    • Updated TanStack Query client package metadata for distribution.
  • Refactor

    • Streamlined internal module dependencies and import structure for improved code organization.

Copilot AI review requested due to automatic review settings November 4, 2025 20:55
@claude
Copy link

claude bot commented Nov 4, 2025

Claude finished @ymc9's task —— View job


PR Review: ESM Compatibility Fix

Reviewed the ESM compatibility changes for tsjapi.

Review Checklist:

  • Read and analyze changed files
  • Examine ESM compatibility improvements
  • Check code quality and best practices
  • Look for potential bugs or issues
  • Assess performance considerations
  • Review security implications
  • Evaluate test coverage needs

Review Summary

This PR addresses ESM compatibility issues with the ts-japi library by making two key changes:

Changes Made:

  1. packages/clients/tanstack-query/package.json: Removed "private": true

    • This change allows the package to be published, improving distribution
    • The package already has "type": "module" for ESM support
  2. packages/server/src/api/rest/index.ts: Fixed import/export patterns

    • Changed from named imports (Linker, Paginator, etc.) to namespace import (tsjapi)
    • Updated all usages to use tsjapi.Linker, tsjapi.Paginator, etc.

Strengths

  • Correct ESM Fix: The namespace import approach (import tsjapi, { type ... }) is the proper solution for libraries that don't have proper ESM named exports
  • Consistent Implementation: All 8 instances of ts-japi class usage have been consistently updated
  • Type Safety Maintained: Type imports are preserved with the type keyword
  • No Breaking Changes: The changes are internal implementation details that don't affect the public API

⚠️ Observations

  • Root Cause: The ts-japi library (v1.12.0) appears to have CommonJS-style exports that don't work properly with ESM named imports
  • Standard Pattern: Using namespace imports with dot notation is a common workaround for such compatibility issues
  • Publishing Readiness: Removing "private": true from tanstack-query package suggests this package is ready for publication

🔍 No Issues Found

  • Security: No security concerns - only import pattern changes
  • Performance: No performance impact - same functionality with different import syntax
  • Code Quality: Clean, consistent changes following established patterns
  • Type Safety: All type annotations preserved correctly

💡 Recommendations

  1. Test Coverage: Ensure existing tests still pass to verify the import changes don't break functionality
  2. Build Verification: Run the build process to confirm ESM compatibility is resolved
  3. Consider Future: Monitor ts-japi updates for proper ESM support to potentially revert to named imports later

Verdict: ✅ APPROVE

This is a well-executed fix for ESM compatibility issues. The changes are minimal, focused, and use the correct approach for handling libraries with problematic ESM exports. The implementation is consistent and maintains type safety throughout.


Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the ts-japi import pattern and prepares the tanstack-query package for publication. The changes address a module import compatibility issue and make the tanstack-query client publicly available.

  • Changed ts-japi imports from named imports to default import with type-only named imports
  • Updated all ts-japi class usages to reference the default import namespace
  • Removed the "private" flag from the tanstack-query package to allow npm publication

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/server/src/api/rest/index.ts Updated ts-japi import to use default import and prefixed all class instantiations with the namespace
packages/clients/tanstack-query/package.json Removed private flag to enable package publication

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@coderabbitai
Copy link

coderabbitai bot commented Nov 4, 2025

Walkthrough

This PR makes the tanstack-query package publishable by removing its private flag and refactors ts-japi imports in the REST API handler from named imports to default import with type-only imports, updating all internal usages to namespace-qualified calls.

Changes

Cohort / File(s) Summary
Package Metadata
packages/clients/tanstack-query/package.json
Removed top-level "private": true field to make package publishable
Import Refactoring
packages/server/src/api/rest/index.ts
Switched ts-japi imports from named imports (Linker, Paginator, Relator, Serializer, SerializerOptions) to default import (tsjapi) with type-only named imports; updated all internal instantiations (Linker, Paginator, Serializer, Relator) to use tsjapi namespace qualification

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Import refactoring is straightforward with consistent namespace usage pattern applied throughout
  • Package metadata change is cosmetic and low-risk

Poem

🐰 A package now sees the light of day,
Private walls crumble away,
With ts-japi in namespace dress,
Imports refined, code looks its best! ✨

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: tsjapi esm compatibility issue' directly relates to the main change in the PR, which involves reworking ts-japi imports (tsjapi) to switch from named imports to default + type-only imports, a typical fix for ESM compatibility.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/tsjapi

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5dc0087 and 95b2762.

📒 Files selected for processing (2)
  • packages/clients/tanstack-query/package.json (0 hunks)
  • packages/server/src/api/rest/index.ts (8 hunks)
💤 Files with no reviewable changes (1)
  • packages/clients/tanstack-query/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.

Applied to files:

  • packages/server/src/api/rest/index.ts
⏰ 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). (2)
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (2)
packages/server/src/api/rest/index.ts (2)

613-615: All ts-japi constructor calls correctly updated to namespace pattern.

All instantiations of Linker, Paginator, Serializer, and Relator have been systematically updated to use the tsjapi namespace prefix. The changes are consistent throughout the file and maintain the same functionality while addressing ESM compatibility.

Also applies to: 675-677, 832-851, 1115-1117, 1313-1317, 1331-1339, 1365-1383


13-13: Review verification inconclusive — manual confirmation needed.

The code changes are consistently applied throughout the file: all 9 constructor usages correctly reference the tsjapi namespace (lines 613, 675, 832, 1115, 1313, 1331, 1365, 1373, 1376), and type-only imports are properly preserved. However, ts-japi v1.12.0 exports these classes as named exports, which typically follows the standard ESM pattern of import { Serializer, Linker } rather than the hybrid pattern import tsjapi, { type Serializer } used here.

The original review claimed this hybrid pattern is "common" for ESM transitions, but web search results indicate otherwise. This pattern may be specific to how ts-japi structures its exports, but I cannot verify the actual package export field definitively.

Verify that this import pattern is compatible with ts-japi 1.12.0 by running the application tests or importing the package in your project environment to confirm the classes resolve correctly at runtime.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ymc9 ymc9 merged commit b9294bb into dev Nov 4, 2025
11 checks passed
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