Skip to content

fix(graphql-codegen-client-preset): respect namingConvention config#593

Merged
kdy1 merged 1 commit intomainfrom
claude/issue-521-20260321-0028
Mar 21, 2026
Merged

fix(graphql-codegen-client-preset): respect namingConvention config#593
kdy1 merged 1 commit intomainfrom
claude/issue-521-20260321-0028

Conversation

@kdy1
Copy link
Copy Markdown
Member

@kdy1 kdy1 commented Mar 21, 2026

Fixes #521

Add namingConvention option to the SWC plugin to match the naming convention configured in codegen.ts. The default is now change-case-all#pascalCase (matching graphql-codegen's client-preset default), which correctly handles word boundaries in names like SomeEGRocketsSomeEgRocketsDocument.

Previously, the plugin used upperCaseFirst behavior (only capitalize first letter), causing mismatches when operation names contained consecutive uppercase letters.

Generated with Claude Code

Add `namingConvention` option to the SWC plugin to match the naming
convention configured in codegen.ts. The default is now
`change-case-all#pascalCase` (matching graphql-codegen's client-preset
default), which correctly handles word boundaries in names like
`SomeEGRockets` → `SomeEgRocketsDocument`.

Previously, the plugin used `upperCaseFirst` behavior (only capitalize
first letter), causing mismatches when operation names contained
consecutive uppercase letters like `SomeEGRockets`.

Fixes #521

Co-authored-by: Donny/강동윤 <kdy1@users.noreply.github.com>
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 21, 2026

⚠️ No Changeset found

Latest commit: 332e70f

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

@kdy1 kdy1 enabled auto-merge (squash) March 21, 2026 00:37
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@claude
Copy link
Copy Markdown

claude Bot commented Mar 21, 2026

Code Review

Thanks for this fix! The issue is real and the approach is generally sound. Here's my feedback:

Positive changes

  • Fixes the typo capetalizeupper_case_first and improves the implementation to use chars() instead of byte-slicing (&s[..1]). The old code would panic on empty strings or multi-byte UTF-8 characters, so this is an important correctness fix.
  • The to_pascal_case word-boundary detection logic is correct. I traced through SomeEGRockets:
    • e→E triggers lower→upper boundary → "Some"
    • G→R (uppercase+uppercase followed by lowercase) triggers the third condition → "EG"
    • Remaining: "Rockets"
    • Result: "SomeEgRockets"

Concerns

1. Missing changeset

The changeset bot flagged this. This is a breaking change in default behavior — the default naming convention is changing from upperCaseFirst to pascalCase. Existing users whose operation names contain consecutive uppercase letters (like SomeEGRockets) will get different import names after upgrading, which will break their builds. This warrants at minimum a minor version bump with clear release notes, not just a patch.

2. Silent fallback for unrecognized conventions

fn apply_naming_convention(s: &str, naming_convention: &str) -> String {
    match naming_convention {
        "change-case-all#upperCaseFirst" => upper_case_first(s),
        _ => to_pascal_case(s),  // ← silently handles unknown conventions
    }
}

If a user sets namingConvention: "change-case-all#camelCase" (or any other valid graphql-codegen convention), they'll silently get pascal case instead. This could cause confusing build failures with no clear error. Consider returning an error or at least logging a warning for unrecognized conventions.

3. Re-implementing change-case-all behavior

Rather than re-implementing the pascal case algorithm, have you considered depending on the convert_case Rust crate? That said, I understand SWC WASM plugins sometimes have restricted dependency options, so this may not be practical — worth checking though.

4. Limited convention support not documented

The README only mentions pascalCase and upperCaseFirst, but doesn't state that other conventions are unsupported. Users who try camelCase or snakeCase will silently get incorrect behavior. Please document the supported values explicitly.

5. Edge cases in to_pascal_case not covered by tests

The function handles these correctly on inspection, but tests would make this more robust:

  • Empty string: """"
  • Single character: "A""A"
  • Leading/trailing non-alphanumeric: "_SomeQuery_""SomeQuery"
  • All uppercase: "ALLCAPS""Allcaps" (is this the desired behavior?)
  • Numbers: "Query123Name" — digits are treated as alphanumeric, no boundary between digits and adjacent letters

6. Minor: unnecessary clone()

let import_name = apply_naming_convention(&operation_name, &self.options.naming_convention);
self.graphql_operations_or_fragments_to_import.push(import_name.clone());
let new_expr = Expr::Ident(quote_ident!(import_name).into());

The clone() is fine here, but you could push after constructing new_expr to avoid it, or use import_name.as_str() for the ident and push the original.

Summary

The core fix is correct and needed. The main action items before merging:

  1. Add a changeset (and consider whether this is minor/breaking rather than patch)
  2. Handle or document unsupported naming conventions
  3. Update the README to list supported namingConvention values explicitly

Generated with Claude Code

@kdy1 kdy1 merged commit a6f0c50 into main Mar 21, 2026
10 of 11 checks passed
@kdy1 kdy1 deleted the claude/issue-521-20260321-0028 branch March 21, 2026 00:40
Copy link
Copy Markdown

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

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: 332e70f974

ℹ️ 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 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 address that feedback".

Comment on lines +84 to +87
fn apply_naming_convention(s: &str, naming_convention: &str) -> String {
match naming_convention {
"change-case-all#upperCaseFirst" => upper_case_first(s),
_ => to_pascal_case(s),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Handle non-pascal namingConvention values instead of forcing PascalCase

apply_naming_convention only recognizes change-case-all#upperCaseFirst; every other string falls through to to_pascal_case. GraphQL Code Generator also accepts values like keep, change-case-all#lowerCase, lodash#camelCase, or a custom module path, so projects that mirror one of those valid configs into this plugin will still import PascalCased symbols. For example, with namingConvention: "keep", an operation like SomeEGRockets should stay SomeEGRocketsDocument, but this transform rewrites it to SomeEgRocketsDocument, which no longer matches the generated export.

Useful? React with 👍 / 👎.

Comment on lines +321 to +322
#[serde(default = "naming_convention_default")]
namingConvention: String,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Accept object-form namingConvention configs

PluginOptions now deserializes namingConvention as a plain String, but valid Codegen configs also use the object form (for example to set enumValues separately or enable transformUnderscore). If a project follows the new README guidance and passes that same config into the SWC plugin, serde_json::from_str(...).expect(...) in process_transform will reject the object and abort the transform before compilation starts.

Useful? React with 👍 / 👎.

kdy1 added a commit that referenced this pull request Mar 21, 2026
kdy1 added a commit that referenced this pull request Mar 21, 2026
## Summary
- Addressed `chatgpt-codex-connector[bot]` review follow-ups across
merged PRs: #585, #591, #592, #593, #594, #595, #597, #598, #599.
- Applied the fixes as 9 separate commits (one commit per original PR)
in a single follow-up branch.

## What Changed
- #585 / #592 (`loadable-components`)
- Fixed source-less default-import matching to honor the configured
local name.
- Updated `ssr: false` detection to respect final object-literal
override order.
  - Added/updated fixtures for both behaviors.
- #591 / #594 (`formatjs`)
- Added JSX member-expression message component support (e.g.
`ReactIntl.FormattedMessage`).
  - Updated #532 regression coverage to validate the `ast: true` path.
- #593 / #595 (`graphql-codegen-client-preset`)
  - Extended `namingConvention` parsing to accept string/object forms.
- Preserved names for `keep`/unknown conventions instead of forcing
PascalCase.
- Fixed Windows absolute `filename` path handling in WASM runtime path
resolution.
  - Added unit coverage for the new config/path behaviors.
- #597 / #599 (`emotion`)
- Ensured tagged-template labels are terminated before sourcemap
comments.
- Added css-prop rewrite support for namespace imports
(`emotionReact.css`).
  - Attached PURE comments to the generated call site span.
  - Updated emotion fixtures accordingly.
- #598 (docs)
- Corrected capability descriptions in `packages/jest/README.tmpl.md`
and `packages/swc-sdk/README.tmpl.md`.

## Validation
- `cargo test -p swc_plugin_loadable_components --test fixture --
--ignored`
- `cargo test -p swc_plugin_graphql_codegen_client_preset`
- `cargo test -p swc_emotion --test fixture -- --ignored`
- `pnpm -C /Users/kdy1/.codex/worktrees/17e6/plugins/packages/formatjs
test`

All passed (formatjs has an existing non-blocking Vitest warning about
an un-awaited rejects assertion).
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.

graphql-codegen-client-preset doesn't respect codegen.ts namingConvention

2 participants