Skip to content

fix(graphql-codegen): preserve digit suffix pascal case - #637

Merged
Donny/강동윤 (kdy1) merged 1 commit into
mainfrom
kdy1/fix-graphql-codegen-digit-suffix
Jun 30, 2026
Merged

fix(graphql-codegen): preserve digit suffix pascal case#637
Donny/강동윤 (kdy1) merged 1 commit into
mainfrom
kdy1/fix-graphql-codegen-digit-suffix

Conversation

@kdy1

Copy link
Copy Markdown
Member

Summary

Fixes #630.

This updates the GraphQL codegen client preset plugin's PascalCase normalization so a digit-to-letter transition is treated as a word boundary. That keeps generated import names aligned with @graphql-codegen/client-preset for names such as Hero30FragmentDoc and Foo1Document.

Changes

  • Add a digit -> letter boundary rule in to_pascal_case.
  • Add direct naming-convention assertions for digit-suffixed fragment and operation names.
  • Add an SWC transform snapshot covering both fragment Hero30 and query Foo1.

Validation

  • cargo test -p swc_plugin_graphql_codegen_client_preset --lib

@changeset-bot

changeset-bot Bot commented Jun 30, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 693c1be

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review: fix(graphql-codegen): preserve digit suffix pascal case

Thanks for the fix — the change is small, focused, well-commented, and comes with good test coverage (both a direct apply_naming_convention unit test and an end-to-end SWC transform snapshot). The regression guard asserting SomeEGRocketsDocumentSomeEgRocketsDocument is a nice touch to ensure the new boundary rule doesn't disturb the existing uppercase-sequence handling.

One correctness concern: is_alphabetic() over-splits vs. change-case-all

The new rule treats any digit → letter transition as a word boundary:

} else if chars[i - 1].is_numeric() && chars[i].is_alphabetic() {
    // number → letter (e.g., Hero30Fragment: 0→F)
    true
}

to_pascal_case is emulating @graphql-codegen's change-case-all#pascalCase, which under the hood uses change-case's default split regexp ([a-z0-9])([A-Z]) — i.e. it only splits a digit when it's followed by an uppercase letter. A digit followed by a lowercase letter is not a boundary there.

Because this rule uses is_alphabetic() (which also matches lowercase), it diverges for digit→lowercase inputs:

  • change-case-all#pascalCase("abc123def")Abc123def
  • this code → Abc123Def

A more faithful match would be to gate on uppercase, mirroring the existing lowercase → uppercase arm:

} else if chars[i - 1].is_numeric() && chars[i].is_uppercase() {
    // number → uppercase letter (e.g., Hero30Fragment: 0→F)
    true
}

This still passes all three cases added in this PR (Hero30FragmentDoc, Foo1Document, SomeEGRocketsDocument) since each has an uppercase letter after the digit, while avoiding the over-split on digit→lowercase names. Whether digit→lowercase identifiers actually surface from codegen is admittedly an edge case, but tightening the predicate keeps the function's contract aligned with the library it's mimicking and is essentially free.

Suggestion: add a digit→lowercase test

If you make the change above (or even to lock in current behavior), a test like apply_naming_convention("Foo1bar", "change-case-all#pascalCase") would document the intended digit→lowercase handling and prevent future regressions.

Other notes

  • Performance / security: no concerns — the function operates on short identifier strings and the added branch is O(1) per char.
  • Style: consistent with the surrounding code and comment conventions. 👍

Overall this is a solid, well-tested fix for #630. The only actionable item is the is_alphabetic() vs is_uppercase() faithfulness point above.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8efab6428

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

} else if chars[i - 1].is_lowercase() && chars[i].is_uppercase() {
// lowercase → uppercase (e.g., someEG: e→E)
true
} else if chars[i - 1].is_numeric() && chars[i].is_alphabetic() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid splitting digits before lowercase letters

When a valid GraphQL operation or fragment name contains a digit followed by a lowercase letter, such as query Foo1bar, this new condition treats the 1b transition as a word boundary. The transform then converts the appended name Foo1barDocument to Foo1BarDocument, while change-case-all#pascalCase leaves that lowercase continuation intact and only needs the digit→uppercase split for suffixes like Document/FragmentDoc; this makes the plugin import a symbol that the generated graphql.ts does not export.

Useful? React with 👍 / 👎.

@kdy1
Donny/강동윤 (kdy1) force-pushed the kdy1/fix-graphql-codegen-digit-suffix branch from f8efab6 to 693c1be Compare June 30, 2026 08:46
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review: fix(graphql-codegen): preserve digit suffix pascal case

Thanks for the fix! I reviewed the change and ran cargo test -p swc_plugin_graphql_codegen_client_preset --lib locally — all 16 tests pass. This is a clean, well-scoped fix.

Correctness ✅

The new digit → uppercase letter boundary rule is the right approach and matches the reference behavior in @graphql-codegen/client-preset (which relies on change-case). For context, change-case's splitter inserts word boundaries on the pattern [a-z0-9][A-Z] (plus the [A-Z][A-Z][a-z] acronym rule). The previous code only handled the [a-z][A-Z] half of that first pattern; this PR completes it by adding the [0-9][A-Z] half. So:

  • Hero30FragmentDocHero30FragmentDoc (was Hero30fragmentDoc) ✅
  • Foo1DocumentFoo1Document

The branch is correctly placed as a mutually-exclusive else if after the existing lowercase→uppercase rule, so no ordering concerns. I also confirmed there's no regression risk: digit→lowercase transitions remain non-boundaries (verified by the Foo1barDocument → Foo1barDocument assertion), which matches change-case not splitting [0-9][a-z].

Test coverage ✅

Good coverage — both a direct apply_naming_convention unit test and an end-to-end SWC transform snapshot. Nice touch including:

  • Foo1barDocument as a negative case (digit→lowercase stays joined), and
  • SomeEGRocketsDocument to guard that the acronym rule still works alongside the new digit rule.

Minor / optional nits

  1. is_numeric() vs is_ascii_digit(): char::is_numeric() is Unicode-aware (matches superscripts, fractions, other-script digits) whereas change-case only splits on ASCII 0-9. In practice this is a non-issue here because GraphQL names are restricted to /[_A-Za-z][_0-9A-Za-z]*/ (ASCII only), so they can never contain a non-ASCII numeric. Still, is_ascii_digit() would more precisely mirror the reference semantics and document the intent — purely optional, and consistent with the existing is_lowercase/is_uppercase usage which has the same theoretical Unicode gap.

  2. The inline comment // number → uppercase letter (e.g., Hero30Fragment: 0→F) is clear and consistent with the neighboring comments. 👍

Summary

Correct, minimal, and well-tested fix that aligns the plugin with the upstream client-preset naming. No blocking concerns. LGTM. 🚀

@kdy1
Donny/강동윤 (kdy1) merged commit c7cd20a into main Jun 30, 2026
11 checks passed
@kdy1
Donny/강동윤 (kdy1) deleted the kdy1/fix-graphql-codegen-digit-suffix branch June 30, 2026 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

to_pascal_case lowercases the suffix for names ending in a digit (e.g. Hero30Hero30fragmentDoc)

1 participant