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 x-enum-varnames support #405

Closed
wants to merge 5 commits into from

Conversation

goodoldneon
Copy link

Currently, json-schema-to-typescript uses a custom tsEnumNames extension for naming enum members. However, this extension is specific only to this library. Other major OpenAPI generators (1, 2) have adopted x-enum-varnames for the same purpose, so this PR brings json-schema-to-typescript in line with them.

This PR addresses feature request #200.

Copy link
Owner

@bcherny bcherny left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Can you also add a normalizer step to convert tsEnumNames -> x-enum-varnames, and throw if the latter is already defined and different from what we would have generated?

That will also let you simplify your changes to parser.ts a tiny bit.

@goodoldneon
Copy link
Author

@bcherny Would I add the following rule:

rules.set('Convert tsEnumNames -> x-enum-varnames', schema => {
  if (schema.tsEnumNames) {
    schema['x-enum-varnames'] = schema.tsEnumNames
    delete schema.tsEnumNames
  }
})

And then be able to always reference ['x-enum-varnames'], even when the spec actually uses .tsEnumNames?

@bcherny
Copy link
Owner

bcherny commented Sep 12, 2021

Yup, exactly. Don't forget to validate when you do that too, to make sure the schema doesn't already have conflicting x-enum-varnames and tsEnumNames defined.

@goodoldneon
Copy link
Author

Is there a way to log a deprecation warning so that consumers know tsEnumNames may eventually be removed?

@goodoldneon
Copy link
Author

@bcherny are you OK with the way I'm handling the existence of both x-enum-varnames and tsEnumNames? I figured throwing an error would be fine

Copy link
Owner

@bcherny bcherny left a comment

Choose a reason for hiding this comment

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

Looking better -- almost there!

rules.set('Convert tsEnumNames -> x-enum-varnames', schema => {
if (schema.tsEnumNames) {
if (Object.keys(schema).includes('x-enum-varnames')) {
throw new Error('Cannot set both x-enum-varnames and tsEnumNames in the same schema. Only use x-enum-varnames.')
Copy link
Owner

Choose a reason for hiding this comment

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

Mind being a little more lenient, and only throwing if the two don't match? These should also have tests, if possible.

ie.

// ok
{
  tsEnumNames: ['a']
}

// ok
{
  x-enum-varnames: ['a']
}

// ok
{
  tsEnumNames: ['a'],
  x-enum-varnames: ['a']
}

// error
{
  tsEnumNames: ['a'],
  x-enum-varnames: []
}

// error
{
  tsEnumNames: [],
  x-enum-varnames: ['a']
}

// error
{
  tsEnumNames: ['a'],
  x-enum-varnames: ['b']
}

// error
{
  tsEnumNames: ['a'],
  x-enum-varnames: ['a', 'b']
}

@@ -93,10 +97,18 @@ export interface EnumJSONSchema extends NormalizedJSONSchema {
enum: any[]
}

export interface NamedEnumJSONSchema extends NormalizedJSONSchema {
interface DeprecatedNamedEnumJSONSchema extends NormalizedJSONSchema {
'x-enum-varnames'?: never
tsEnumNames: string[]
Copy link
Owner

Choose a reason for hiding this comment

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

Normalized schemas shouldn't contain tsEnumNames, since you're normalizing it away.

Copy link
Author

@goodoldneon goodoldneon Sep 25, 2021

Choose a reason for hiding this comment

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

So I should delete DeprecatedNamedEnumJSONSchema and rename _NamedEnumJSONSchema to NamedEnumJSONSchema?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah just add x-enum-varnames: string[] and tsEnumNames: never to NormalizedJSONSchema. No need for new types if you can avoid it.

/**
* `tsEnumNames` is being replaced by `x-enum-varnames`, but we'll leave this
* validator until `tsEnumNames` is removed.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Please make sure this is still covered by tests.

Copy link
Author

Choose a reason for hiding this comment

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

Should I copy enumValidation.1.ts and enumValidation.2.ts, but sed "s/tsEnumNames/x-enum-varnames/" in the new test files?

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds ok to me, or merge into a single test file that covers all the cases (x-, ts-, x- and ts-, etc.).

/**
* `tsEnumNames` is being replaced by `x-enum-varnames`, but we'll leave this
* validator until `tsEnumNames` is removed.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Please make sure this is still covered by tests.

@galah92
Copy link

galah92 commented May 11, 2022

Hi, is there any chance it'll get merged soon?

@goodoldneon
Copy link
Author

@galah92 I have a couple open questions for @bcherny. I can pick this back up when he answers

@@ -93,10 +97,18 @@ export interface EnumJSONSchema extends NormalizedJSONSchema {
enum: any[]
}

export interface NamedEnumJSONSchema extends NormalizedJSONSchema {
interface DeprecatedNamedEnumJSONSchema extends NormalizedJSONSchema {
'x-enum-varnames'?: never
tsEnumNames: string[]
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah just add x-enum-varnames: string[] and tsEnumNames: never to NormalizedJSONSchema. No need for new types if you can avoid it.

/**
* `tsEnumNames` is being replaced by `x-enum-varnames`, but we'll leave this
* validator until `tsEnumNames` is removed.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Sounds ok to me, or merge into a single test file that covers all the cases (x-, ts-, x- and ts-, etc.).

@bcherny
Copy link
Owner

bcherny commented May 23, 2022

@galah92 I have a couple open questions for @bcherny. I can pick this back up when he answers

Sorry I missed this! The status was Changes Requested, so I missed your comments.

@goodoldneon
Copy link
Author

No worries, @bcherny!

I started taking a look at this PR again and I'm having a hard time understanding my changes 😅. I think it'd take some effort to ramp myself back up, unfortunately. If this is something you could bang out quickly, then that's probably best. But if that's not the case then I can try to work on this again

@bcherny
Copy link
Owner

bcherny commented May 4, 2023

Closing out stale PRs. If you'd like to revisit this, feel free to re-open.

@bcherny bcherny closed this May 4, 2023
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

3 participants