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

fix(enum-values): access optional prop child props #2139

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

darkbasic
Copy link

@darkbasic darkbasic commented Feb 5, 2025

Changes

Fixes #2138 and #2140

How to Review

How can a reviewer review your changes? What should be kept in mind for this review?

Checklist

  • Unit tests updated
  • docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

@darkbasic darkbasic requested a review from a team as a code owner February 5, 2025 16:10
@darkbasic darkbasic requested a review from drwpow February 5, 2025 16:10
Copy link

netlify bot commented Feb 5, 2025

👷 Deploy request for openapi-ts pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit b2e45cd

Copy link

changeset-bot bot commented Feb 5, 2025

⚠️ No Changeset found

Latest commit: b2e45cd

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

@darkbasic
Copy link
Author

I've committed some further modifications to handle corner cases like the following that I've found in my codebase:

FormFieldBulkStoreRequest: {
    data: {
        composite_fields?: {
            type: "TextSingleLine" | "TextMultiLine" | "Integer" | "Boolean" | "Date" | "Datetime" | "Reference" | "Asset";
        }[] | null;
    }[] | null;
};

@duncanbeevers
Copy link
Contributor

duncanbeevers commented Feb 5, 2025

I'm not for this approach.

Currently we export consts which reference into the existing types.
This PR continues with that approach, layering on a utility type to deal with the complexity of keying into those existing types.

Instead, the consts themselves should represent the basis for the type, and the paths / operations / components structures should reference the types derived from those concrete values.

This is the approach I've already taken in #2051, so I'll prioritize making those changes backwards-compatible with the 7.x branch, which should address both #2138 and #2140.

@darkbasic
Copy link
Author

darkbasic commented Feb 5, 2025

I definitely prefer your approach, I just aimed for the easiest and quickest solution.
Does #2051 fix #941? The enum could be used to compute the union types used in paths / operations / components from its keys.

@duncanbeevers
Copy link
Contributor

I think #2051 doesn't address #941 as it expresses the concrete values as const array, and side-steps indexed access.
However, the plan is to change the representation to a const object with symmetric keys + values, and one driver of that is the desire to enable the namespace-like access.

There's still open discussion about naming.
One possibility is overloading the names, (eg; Prisma approach)
Another is exposing the type and the concrete value with distinct names.

@darkbasic
Copy link
Author

@duncanbeevers why don't we base everything on top of the enum?

export enum StatusEnum {
  ACTIVE = 'active',
  INACTIVE = 'inactive'
}
export type Status = `${StatusEnum}`
export const StatusValues: Status[] = Object.values(StatusEnum)
export interface components {
  schemas: {
    Status: Status
  }
}

The approach is the same of your PR but you get #941 for free.

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.

--enum-values tries to access optional prop child props in order to define the array type
2 participants