Skip to content

fix: stop pascal casing enums for MIS #931

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 18, 2025
Merged

fix: stop pascal casing enums for MIS #931

merged 4 commits into from
Feb 18, 2025

Conversation

atierian
Copy link
Contributor

@atierian atierian commented Feb 10, 2025

Description of changes

Codegen currently pascal cases enums defined in the GraphQL schema.

Gen 2 doesn't pascal case top-level enums:

const schema = a.schema({
  foo: a.enum(["a1", "b1"]),
});

generates:

enum foo {
  a1
  b1
}

This naming discrepancy hasn't caused problems yet because:

  1. Type names for return types aren’t included in GraphQL operations.
  2. The only way an enum can be used as an input today is through a generated model query. For example:
const schema = a.schema({
  foo: a.enum(["a1", "b1"]),
  Todo: a.model({
    status: a.boolean(),
    test: a.ref('foo')
  }),
});

But we create a wrapper input type for all model generated queries. So the actual GraphQL mutation here uses CreateTodoInput + Variables and never mentions the foo type.

{
  "query": "mutation ($input: CreateTodoInput!) {
    createTodo(input: $input) {
      id
      status
      test
      createdAt
      updatedAt
    }
  }",
  "variables": {
    "input": {
        "status": true,
        "test": "b1"
    }
  }
}

This changes with the custom type / enum input for custom operation work in progress.

const schema = a.schema({
  foo: a.enum(["a1", "b1"]),
  fcnCall: a
    .query()
    .arguments({
      e3: a.ref("foo"),
    }),
});

will generate a GraphQL query like:

{
  "query": "query ($e3: Foo) {
    fcnCall(e3: $e3)
  }",
  "variables": {
    "e3": "a1"
  }
}

because there isn’t a wrapper input type.

Note

This change only applies to the model introspection schema.
Codegenerated enums are still pascal cased because changing that will cause a breaking change if the schema defined enum casing doesn't match. Non-MIS consuming clients (e.g. native codegen) doesn't support custom operations, so the feature motivating this change doesn't apply.

Codegen Paramaters Changed or Added

Issue #, if available

N/A

Description of how you validated changes

Note

Two Gen 2 related E2Es are failing in this run, but they have been failing for a long time and the failures are unrelated. I'm unsure of when the change causing these failures was introduced. The only way I've been able to repro the failure locally is by running yarn e2e ... instead of npm e2e ..., which results in both yarn.lock and package-lock.json files being generated in the project root. This in turn causes an error when deploying and mimics the observed failures in CloudBuild runs.

Checklist

  • PR description included
  • yarn test passes
  • E2E test run linked
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • Breaking changes to existing customers are released behind a feature flag or major version update
  • Changes are tested using sample applications for all relevant platforms (iOS/android/flutter/Javascript) that use the feature added/modified
  • Changes are tested on windows. Some Node functions (such as path) behave differently on windows.
  • Changes adhere to the GraphQL Spec and supports the GraphQL types type, input, enum, interface, union and scalar types.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@atierian atierian requested a review from a team as a code owner February 10, 2025 21:35
@@ -305,7 +305,7 @@ export class AppSyncSwiftVisitor<
.asKind('enum')
.access('public')
.withProtocols(['String', 'EnumPersistable'])
.withName(this.getEnumName(enumValue));
.withName(pascalCase(this.getEnumName(enumValue)));
Copy link

Choose a reason for hiding this comment

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

Is there a swift test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -143,6 +147,10 @@ describe('Model Introspection Visitor', () => {
expect((visitor as any).getType('SimpleInterface')).toEqual({ interface: 'SimpleInterface' });
});

it('should not pascal case enum', () => {
expect((visitor as any).getType('camel')).toEqual({ enum: 'camel' })
Copy link

Choose a reason for hiding this comment

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

Can we test with a two word name like camelCase to ensure that Case is still capitalized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in aea768b

@atierian atierian merged commit f763085 into main Feb 18, 2025
4 checks passed
@atierian atierian deleted the unpascal-enums branch February 18, 2025 18:24
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.

3 participants