Skip to content
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

Add new configuration option to preserve javascript enum names #356

Closed

Conversation

florence-wolfe
Copy link

👋 My team and I really want to use JavaScript enums but we also want to preserve the original enum name as closely as possible.

This PR introduces a new javascript-preserve-name configuration option for the enums field. When selected it will go through a slightly different path in the models code to only strip out invalid characters and unescape the name. Beyond that it will leave the name as-is, preserving casing and valid characters.

For example, given the following enum found in an openapi schema:

            "UPPER_SNAKE_ENUM": {
                "description": "This is a simple enum with a non-PascalCase name.",
                "enum": ["UPPER_SNAKE_0", "UPPER_SNAKE_1", "UPPER_SNAKE_2"],
                "x-enum-varnames": [0, 1, 2],
                "x-enum-descriptions": ["UPPER_SNAKE_0", "UPPER_SNAKE_1", "UPPER_SNAKE_2"]
            },

This would then output the following:

with javascript-preserve-output

/**
 * This is a simple enum with a non-PascalCase name.
 */
export type UPPER_SNAKE_ENUM = 'UPPER_SNAKE_0' | 'UPPER_SNAKE_1' | 'UPPER_SNAKE_2';

export const UPPER_SNAKE_ENUM = {
    /**
     * UPPER_SNAKE_0
     */
    UPPER_SNAKE_0: 'UPPER_SNAKE_0',
    /**
     * UPPER_SNAKE_1
     */
    UPPER_SNAKE_1: 'UPPER_SNAKE_1',
    /**
     * UPPER_SNAKE_2
     */
    UPPER_SNAKE_2: 'UPPER_SNAKE_2'
} as const;

with the original javascript option:

/**
 * This is a simple enum with a non-PascalCase name.
 */
export type UPPER_SNAKE_ENUM = 'UPPER_SNAKE_0' | 'UPPER_SNAKE_1' | 'UPPER_SNAKE_2';

export const UPPERSNAKEENUMEnum = {
    /**
     * UPPER_SNAKE_0
     */
    UPPER_SNAKE_0: 'UPPER_SNAKE_0',
    /**
     * UPPER_SNAKE_1
     */
    UPPER_SNAKE_1: 'UPPER_SNAKE_1',
    /**
     * UPPER_SNAKE_2
     */
    UPPER_SNAKE_2: 'UPPER_SNAKE_2',
} as const;

and with the typescript option:

/**
 * This is a simple enum with a non-PascalCase name.
 */
export enum UPPER_SNAKE_ENUM {
    /**
     * UPPER_SNAKE_0
     */
    UPPER_SNAKE_0 = 'UPPER_SNAKE_0',
    /**
     * UPPER_SNAKE_1
     */
    UPPER_SNAKE_1 = 'UPPER_SNAKE_1',
    /**
     * UPPER_SNAKE_2
     */
    UPPER_SNAKE_2 = 'UPPER_SNAKE_2',
}
  • snapshots have been updated and a test new output test case for the upper snake was included

Copy link

changeset-bot bot commented Apr 11, 2024

🦋 Changeset detected

Latest commit: d315257

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
openapi-ts-docs Patch
@hey-api/openapi-ts Minor

Not sure what this means? Click here to learn what changesets are.

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

Copy link

vercel bot commented Apr 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hey-api-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 11, 2024 3:26pm

@mrlubos
Copy link
Member

mrlubos commented Apr 11, 2024

Hi @florence-wolfe, thank you for the pull request! Can you clarify why you want to preserve the original enum name?

@florence-wolfe
Copy link
Author

florence-wolfe commented Apr 11, 2024

In our particular case we have enums such as APP_NAME that are currently used in both our FE and BE monorepos, however, we're moving toward generated types to ensure a single source of truth for the schemas. That said, we want to preserve the existing nomenclature as they can represent business rules and organization-wide class words.

But beyond that, different organizations and projects have different style guides for variable and enum naming conventions.

@mrlubos
Copy link
Member

mrlubos commented Apr 11, 2024

If I propose changes to this pull request, would you be open to doing them or want me to do them?

@florence-wolfe
Copy link
Author

Absolutely feel free to leave any kinds of comments. This should be collaborative ☺️

@mrlubos
Copy link
Member

mrlubos commented Apr 11, 2024

Love it @florence-wolfe! For context, we're trying to balance asks from different people with maintaining a healthy codebase. My take is we should be opinionated whenever possible to avoid guiding people down the wrong path. For example, allowing people to use positional arguments was a decision that's hard to migrate from and harder to maintain than if useOptions was the only choice. With that being said, this pull request shouldn't pose a similar risk, so we will get it in.

Couple of thoughts:

  • for now, I'd prefer to extend the enums config type to optionally accept an object with values such as { format: 'javascript', preserveNames: true } (feel free to propose key names). That would keep the API surface smaller while supporting this feature. I am a bit concerned about supporting this from CLI, but maybe once you get into advanced configuration, it's okay to require using a config file? Do you personally use a config file?
  • I'd update the docs referencing this feature. The goal is to get people started fast, and only if they don't like the default output should they reach for advanced configuration. Docs related to this feature should be at the end of Enums section, so people don't have to read through it if they're just looking to generate enums as recommended. I'd possibly even make this section hidden behind accordion so it's not taking up space if someone is trying to read through the docs quickly. Too much text can be overwhelming too!
  • I think your preserved variant would strip non-ascii characters, is that correct? I'd like to keep those in as they are a valid JavaScript syntax (see generated schemas) and we have non-English users.

What are your thoughts? Separately, I'm curious what made you use @hey-api? Thanks again!

@mrlubos mrlubos self-assigned this Apr 11, 2024
@mrlubos
Copy link
Member

mrlubos commented Apr 11, 2024

We could also consider preserving names being the only option. The challenge with that is that TypeScript variant will conflict with generated types if they have the same identifier, hence the Enum suffix. I'd like to move enums to a separate file at some point and treat them as its own artefact, that would resolve that problem.

@mrlubos
Copy link
Member

mrlubos commented Apr 11, 2024

@florence-wolfe I am going to move enums to a separate file so we are not blocked on conflicts and we can figure out what to do about the rest of open questions when you're around

EDIT: done

@mrlubos
Copy link
Member

mrlubos commented Apr 12, 2024

@florence-wolfe got a pull request for moving enums into their own file #358

I am not opposed to enum names being preserved with illegal characters being replaced with underscore

@florence-wolfe
Copy link
Author

👋 thanks for your time and effort @mrlubos . Unfortunately, the change to a separate enum file is going to mean I need to find a new tool since I need a single file output.

I'm going to close this as it's no longer relevant to me nor my team, but by all means feel free to reopen if you have a better approach that aligns better with the architecture here! 👍 All the best!

@mrlubos
Copy link
Member

mrlubos commented Apr 12, 2024

@florence-wolfe Can you explain more about why you need a single file output?

@mrlubos
Copy link
Member

mrlubos commented Apr 15, 2024

@florence-wolfe bump

@florence-wolfe
Copy link
Author

Our team has hundreds of engineers so the least overhead possible was desirable. With that said, we're moving to Protobuf and won't be moving forward with OAS any further.

Thanks again!

@mrlubos
Copy link
Member

mrlubos commented Apr 16, 2024

Got it @florence-wolfe, thank you! Feel free to reach out if you consider OpenAPI in the future

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