feat(openapi-typescript): Optional Export Root Type Aliases #1876

BradHacker · 2024-08-23T15:22:07Z

Changes

Adds the CLI option --root-types to optionally export all of the types from components for convenience.

A dependency on change-case was added given the need to account for various casing in the OpenAPI component schema name (e.g. 'some-type') not always being valid for TypeScript type identifier. (If this dependency can be avoided, I'm open to suggestions). Possible edge cases for this breaking are open for discussion, so please let me know of all the edge cases you can think of!

Relevant Issues:

Previous PR's:

feat: export schemas types at root level #1260

How to Review

Keep in mind this is my first ever time working with the TypeScript Compiler API and AST so any recommendations for a way to write this in a cleaner way is more than welcome.

I've added a GitHub example called github-api-root-types.ts and the update:examples should generate this. Tried to stick to what seems to be standard practices here. If there are other preferred methods of testing/demonstrating this feature, please let me know!

Checklist

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

changeset-bot · 2024-08-23T15:22:13Z

🦋 Changeset detected

Latest commit: f8694f7

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

This PR includes changesets to release 1 package

Name	Type
openapi-typescript	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

BradHacker · 2024-08-23T15:39:33Z

Not sure why the openapi-react-query package is failing to build in CI as openapi-fetch is building properly

drwpow · 2024-08-30T14:02:05Z

Not sure why the openapi-react-query package is failing to build in CI as openapi-fetch is building properly

GitHub Actions has had some minor timeouts/failures in the last week. Nothing major, but it seems a few PRs failed for no reason. Can’t complain about free CI though!

drwpow · 2024-08-30T14:03:37Z

docs/cli.md

@@ -104,21 +104,22 @@ The following flags are supported in the CLI:
 | `--help`                           |       |          | Display inline help message and exit                                                                                |
 | `--version`                        |       |          | Display this library’s version and exit                                                                             |
 | `--output [location]`              | `-o`  | (stdout) | Where should the output file be saved?                                                                              |
-| `--redocly [location]`               |       |          | Path to a `redocly.yaml` file (see [Multiple schemas](#multiple-schemas))                                           |
+| `--redocly [location]`             |       |          | Path to a `redocly.yaml` file (see [Multiple schemas](#multiple-schemas))                                           |


❤️ Thanks for fixing little things like this! It means a lot

Your welcome! Gotta keep things consistent 🙂

drwpow · 2024-08-30T14:10:03Z

packages/openapi-typescript/test/transform/components-object.test.ts

+    pathItems: never;
+}
+export type SomeTypeSchema = components['schemas']['SomeType'];`,
+        options: { ...DEFAULT_OPTIONS, rootTypes: true },


So this is a great start to testing this, and this is exactly what we need! But we will need a lot more tests to handle all of the scenarios from my original requirements:

Conflicts: test that #/components/schemas/Schema-Foo and #/components/schemas/Schemafoo don’t conflict (and many combinations including hyphens and invalid JS characters like leading numbers!). We’ll need to handle:

~~Case sensitivity~~ edit: actually on second thought, it’s fine if a user gets case-sensitive conflicts; that’s on them

Invalid characters: -, ., and / are the most common. Beyond that IMO not too important.

Leading numbers (e.g. 1schema)

ALL of components object, not just #/components/schemas:

#/components/schemas

#/components/responses

#/components/parameters

#/components/requestBodies

⚠️ Make sure that #/components/schemas/Foo doesn’t conflict with #/components/responses/Foo! This is a common one.

We will need all of this to happen in the same PR, because it’s very likely that supporting all this could lead to refactoring and churn based on the implementation (see the last point of #2—conflicts are very likely between all the collections). As-is I like the path you’re on and don’t see any problems in your implementation! But we need to have all of these present and tested before we merge this.

Addressed point 1 with d40a7fd. All conflicting names are suffixed by a _2, _3, etc. as per the original discussion in the previous PR you linked. This behavior is up for discussion. Not sure this is a great developer experience, but at the same time don't feel it warrants an error given the OpenAPI spec permits these things. Given that, figured this should be the compliant portion and work around the limitations of JS to comply to the OpenAPI spec as closely as possible. Maybe possible to just print warnings about conflicts and automatic suffixing if detected as a happy compromise?

Filled out the test to cover the other components properties in 7edf333 and tested for conflicts across them in f64e0e6

Given that, figured this should be the compliant portion and work around the limitations of JS to comply to the OpenAPI spec as closely as possible.

+1

Maybe possible to just print warnings about conflicts and automatic suffixing if detected as a happy compromise?

Yeah I’m open to that, but don’t feel strongly. I think developers will realize if they’re importing the wrong things quickly. The main thing to handle was types silently missing from rootTypes. That would have people raising bug reports. But both warnings, and an awkward _2 let people know “hey your schema has a conflict—you may not have even realized it before” (which is very easy to do in multi-file schemas, which a lot of teams rely on)

drwpow · 2024-09-03T19:14:55Z

packages/openapi-typescript/test/transform/components-object.test.ts

+export type RequestBodyUploadUser = components['requestBodies']['UploadUser'];
+export type HeaderAuth = components['headers']['Auth'];
+export type PathItemUploadUser = components['pathItems']['UploadUser'];`,
+options: { ...DEFAULT_OPTIONS, rootTypes: true },


drwpow · 2024-09-03T19:16:38Z

packages/openapi-typescript/test/transform/components-object.test.ts

+    pathItems: never;
+}
+export type SomeTypeSchema = components['schemas']['SomeType'];`,
+        options: { ...DEFAULT_OPTIONS, rootTypes: true },


Given that, figured this should be the compliant portion and work around the limitations of JS to comply to the OpenAPI spec as closely as possible.

+1

Maybe possible to just print warnings about conflicts and automatic suffixing if detected as a happy compromise?

Yeah I’m open to that, but don’t feel strongly. I think developers will realize if they’re importing the wrong things quickly. The main thing to handle was types silently missing from rootTypes. That would have people raising bug reports. But both warnings, and an awkward _2 let people know “hey your schema has a conflict—you may not have even realized it before” (which is very easy to do in multi-file schemas, which a lot of teams rely on)

drwpow · 2024-09-03T19:18:38Z

A dependency on change-case was added given the need to account for various casing in the OpenAPI component schema name (e.g. 'some-type') not always being valid for TypeScript type identifier. (If this dependency can be avoided, I'm open to suggestions).

Nope! This is fine. Any time a lightweight dependency is added that serves a need is OK. Better to have well-tested code handling a common usecase than write code off-the-shelf.

drwpow

Tests looks great, and this meets all the requirements! 🎉 Thanks for adding; a lot of people will love using this feature.

Be sure to add a minor changeset (see comment) so we can merge & release this!

BradHacker · 2024-09-03T19:45:14Z

Tests looks great, and this meets all the requirements! 🎉 Thanks for adding; a lot of people will love using this feature.

Be sure to add a minor changeset (see comment) so we can merge & release this!

Added a minor changeset with a brief summary of the changes. Wanted this feature for a project and I've already been using this update locally. It cleans up the usage so much when trying to add types to my code! Glad to help everyone out 😄

bradatnist added 4 commits August 23, 2024 10:54

Add root component type aliases

136e1f8

Add tests for root component types

4dd0aaf

Add root types CLI option docs

fb5baea

Add unit test for components root types

11cddd0

BradHacker requested a review from a team as a code owner August 23, 2024 15:22

BradHacker changed the title ~~Optional Export Root Type Aliases~~ feat(openapi-typescript): Optional Export Root Type Aliases Aug 23, 2024

drwpow requested changes Aug 30, 2024

View reviewed changes

BradHacker added 4 commits September 3, 2024 14:45

Add schema name conflict detection

d40a7fd

Add tests for components root types beyond schemas

7edf333

Add cross-component conflict checks

f64e0e6

Update examples

f313c64

BradHacker requested a review from drwpow September 3, 2024 19:10

Merge branch 'main' into add-root-types

29b39e4

drwpow reviewed Sep 3, 2024

View reviewed changes

drwpow approved these changes Sep 3, 2024

View reviewed changes

BradHacker added 2 commits September 3, 2024 15:35

Fix biome linting issues

a59a89f

Add changeset

f8694f7

drwpow merged commit a9cd9aa into openapi-ts:main Sep 3, 2024
8 checks passed

github-actions bot mentioned this pull request Sep 3, 2024

[ci] release #1900

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(openapi-typescript): Optional Export Root Type Aliases #1876

feat(openapi-typescript): Optional Export Root Type Aliases #1876

BradHacker commented Aug 23, 2024 •

edited

Loading

changeset-bot bot commented Aug 23, 2024 •

edited

Loading

BradHacker commented Aug 23, 2024

drwpow commented Aug 30, 2024

drwpow Aug 30, 2024

BradHacker Sep 3, 2024

drwpow Aug 30, 2024 •

edited

Loading

BradHacker Sep 3, 2024

drwpow Sep 3, 2024 •

edited

Loading

drwpow Sep 3, 2024

drwpow Sep 3, 2024 •

edited

Loading

drwpow commented Sep 3, 2024

drwpow left a comment

BradHacker commented Sep 3, 2024

feat(openapi-typescript): Optional Export Root Type Aliases #1876

feat(openapi-typescript): Optional Export Root Type Aliases #1876

Conversation

BradHacker commented Aug 23, 2024 • edited Loading

Changes

How to Review

Checklist

changeset-bot bot commented Aug 23, 2024 • edited Loading

🦋 Changeset detected

BradHacker commented Aug 23, 2024

drwpow commented Aug 30, 2024

drwpow Aug 30, 2024

Choose a reason for hiding this comment

BradHacker Sep 3, 2024

Choose a reason for hiding this comment

drwpow Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

BradHacker Sep 3, 2024

Choose a reason for hiding this comment

drwpow Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

drwpow Sep 3, 2024

Choose a reason for hiding this comment

drwpow Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

drwpow commented Sep 3, 2024

drwpow left a comment

Choose a reason for hiding this comment

BradHacker commented Sep 3, 2024

BradHacker commented Aug 23, 2024 •

edited

Loading

changeset-bot bot commented Aug 23, 2024 •

edited

Loading

drwpow Aug 30, 2024 •

edited

Loading

drwpow Sep 3, 2024 •

edited

Loading

drwpow Sep 3, 2024 •

edited

Loading