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

feat: root types #1599

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

Conversation

enkot
Copy link

@enkot enkot commented Mar 27, 2024

Changes

#1598
Exports component's and path's types from the root:

export type RequestEnterpriseAdminListGlobalWebhooks = paths["/admin/hooks"]["get"]["parameters"];
export type RequestEnterpriseAdminGetGlobalWebhook = paths["/admin/hooks/{hook_id}"]["get"]["parameters"];
...
export type SchemaSimpleUser = components["schemas"]["simple-user"];
export type SchemaInstallation = components["schemas"]["installation"];
...
export type ResponseNotFound = components["responses"]["not_found"];
export type ResponseRequiresAuthentication = components["responses"]["requires_authentication"];
...
export type ParameterPerPage = components["parameters"]["per-page"];
export type ParameterPage = components["parameters"]["page"];
...
export type HeaderLink = components["headers"]["link"];
export type HeaderXRateLimitLimit = components["headers"]["x-rate-limit-limit"];

I reworked the transformers a bit to return an array where the first element is a TypeNode, the second is a list of aliases (TypeAliasDeclaration[]). For each element in the list, a corresponding "friendly name" type will be generated, alongside with paths and components.

Additionally, duplicate alias names for components are suffixed with an incremented number using the renameDuplicates function.

Names and implementation are subject to improve 🙈

But it's an open question which level of nested types for paths should be aliased and what is the most convenient way to use them:

RequestEnterpriseAdminListGlobalWebhooks['query']
// or
RequestEnterpriseAdminListGlobalWebhooksQuery
// or
EnterpriseAdminListGlobalWebhooksQuery
// etc
  • We must have tests for output. Not only testing ideal cases; test for:
    • Illegal JS chars
    • Conflicts (foo.bar vs foo-bar—not only how do they get transformed to PascalCase, but is the output deterministic—do the conflicts resolve the same way regardless of document order every time?)
    • Deep-nested $refs
    • $defs support (does it work with root types?)
  • Test for support for #/components/parameters, #/components/responses, and #/components/responseBodies (and testing for conflicts between all of them)
  • Test for support for operation IDs

How to Review

Run ./bin/cli.js with --root-types flag.

Checklist

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

Copy link

changeset-bot bot commented Mar 27, 2024

⚠️ No Changeset found

Latest commit: f233ded

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

@@ -75,6 +75,7 @@
"del-cli": "^5.1.0",
"esbuild": "^0.20.1",
"execa": "^7.2.0",
"scule": "^1.3.0",
Copy link
Owner

Choose a reason for hiding this comment

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

I’m fine with adding this since it’s tiny, well-documented, and well-written. But this will need to be in dependencies if it’s required for runtime! Otherwise it won’t install, and users will get a “module not found” error

@@ -26,8 +27,9 @@ const PATH_PARAM_RE = /\{[^}]+\}/g;
export default function transformPathsObject(
pathsObject: PathsObject,
ctx: GlobalContext,
): ts.TypeNode {
): [ts.TypeNode, ts.TypeAliasDeclaration[]] {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
): [ts.TypeNode, ts.TypeAliasDeclaration[]] {
): { node: ts.TypeNode, aliases: ts.TypeAliasDeclaration[] } {

This is a personal opinion, but let’s not return tuple types for any functions; those don’t scale well. If a function needs to return multiple things, a named object is much better (same for parameters—named objects are more maintainable than ordered params or tuples)

> = {
paths: transformPathsObject,
webhooks: transformWebhooksObject,
components: transformComponentsObject,
$defs: (node, options) =>
$defs: (node, options) => [
Copy link
Owner

Choose a reason for hiding this comment

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

Another tuple type to remove: let’s change this to an interface instead

@drwpow
Copy link
Owner

drwpow commented Mar 30, 2024

Thanks for opening this! This is off to a great start, but we need the following things:

Required

  1. We must have tests for output. Not only testing ideal cases; test for:
    • Illegal JS chars
    • Conflicts (foo.bar vs foo-bar—not only how do they get transformed to PascalCase, but is the output deterministic—do the conflicts resolve the same way regardless of document order every time?)
    • Deep-nested $refs
    • $defs support (does it work with root types?)
  2. Test for support for #/components/parameters, #/components/responses, and #/components/responseBodies (and—you guessed it—testing for conflicts between all of them)
    • You mentioned content types, but that shouldn’t factor in at all since the top-level keys for all these have “friendly names” and should all generate the same way
  3. Test for support for operation IDs (agree or disagree, people will ask for this feature 😅)

I know it’s a lot to ask, but historically, flags have been implemented with gaps, and it’s led to frustration when a feature doesn’t work as expected. I’d like to avoid this for this flag, and release something that works for everyone and not just a limited usecase. And these problems were all reasons the original 1.x moved away from this naming structure. I think we can solve for these now, though, but it will take just a little more planning and a lot more testing

@enkot enkot marked this pull request as draft April 3, 2024 20:34
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.

None yet

2 participants