Skip to content

Conversation

@bajtos
Copy link
Member

@bajtos bajtos commented Apr 23, 2018

Move the definition of ExtensionValue from openapi-v3-types (where it is no longer used) to openapi-spec-build.

Fix openapi type guards to use a more correct object type instead of ExtensionValue, since the guards are not dealing with any extensions.

Fixa json-to-schema converter to use any instead of ExtensionValue when converting a JSON Schema object to an indexable object.

This is a follow-up for #1265.

Checklist

  • npm test passes on your machine
  • (n/a) New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • (n/a) API Documentation in code was updated
  • (n/a) Documentation in /docs/site was updated
  • (n/a) Affected artifact templates in packages/cli were updated
  • (n/a) Affected example projects in examples/* were updated

@bajtos bajtos added this to the April 2018 milestone Apr 23, 2018
@bajtos bajtos self-assigned this Apr 23, 2018
@bajtos bajtos requested a review from raymondfeng April 23, 2018 09:50
@bajtos bajtos mentioned this pull request Apr 23, 2018
7 tasks
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

LGTM, just curious, any reason to export ExtensionValue from openapi-spec-builder instead of openapi-v3?
And one minor comment.

export function jsonToSchemaObject(jsonDef: JsonDefinition): SchemaObject {
const json = jsonDef as {[name: string]: ExtensionValue}; // gets around index signature error
// tslint:disable-next-line:no-any
const json = jsonDef as {[name: string]: any}; // gets around index signature error
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, any reason changing ExtensionValue to any? I thought this PR aims at importing ExtensionValue from openapi-spec-builder.
Not a big deal though.

Copy link
Member Author

Choose a reason for hiding this comment

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

JSON Definition is a JSON Schema object containing well-known keys and values:

export interface JsonDefinition extends Definition {
  allOf?: JsonDefinition[];
  oneOf?: JsonDefinition[];
  anyOf?: JsonDefinition[];
  items?: JsonDefinition | JsonDefinition[];
  additionalItems?: {
    anyOf: JsonDefinition[];
  };
  enum?: PrimitiveType[] | JsonDefinition[];
  additionalProperties?: JsonDefinition | boolean;
  definitions?: {[definition: string]: JsonDefinition};
  properties?: {[property: string]: JsonDefinition};
}

There are not OpenAPI spec extensions involved AFAICT, therefore we should not be using ExtensionValue type.

Based on the comment in the code, my understanding is that we need to cast from JsonDefinition to a string-key-to-any-value object/map to fix a compiler error in the default case below:

default:
  result[property] = json[property];
  break;

I found a more elegant way how solve this problem by casting property to keyof JsonDefinition, see 8de9b75.


export interface Extendable {
[extension: string]: ExtensionValue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Extendable should be Extensible.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can use ISpecificationExtension instead of Extendable. However, openapi3-ts does not come with any alias type similar to our ExtensionValue, that's why I thought it's better to preserve both our types.

OTOH, this is not a big deal. I think the benefits of using ISpecificationExtension are worth it.

I have updated the code accordingly, see a9e4ab9.

@bajtos bajtos force-pushed the refactor/extension-value branch from d8fef35 to a9e4ab9 Compare April 24, 2018 10:52
@bajtos
Copy link
Member Author

bajtos commented Apr 24, 2018

any reason to export ExtensionValue from openapi-spec-builder instead of openapi-v3?

@jannyHou since ExtensionValue is not used anywhere in openapi-v3, I feel it's better to leave it out and keep openapi-v3 package smaller. Ideally, this type alias for any should be defined in openapi3-ts.

Based on Raymond's feedback, I removed this type alias entirely.


@raymondfeng @jannyHou PTAL again.

I'll clean up the git commit history (squash all commits to a single one) after the patch gets approved.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

any or any alias are both fine to me, as long as the usages are identical across all packages.
👍 LGTM!

Remove the definition of ExtensionValue from openapi-v3-types as it
is no longer used.

Remove Extendable type from openapi-spec-builder and use
ISpecificationExtension from openapi3-ts instead.

Fix openapi type guards to use a more correct `object` type instead of
`ExtensionValue`, since the guards are not dealing with any extensions.

Fix json-to-schema converter to use a different workaround
for the compiler error related to accessing index properties
on a type without any indexer.
@bajtos bajtos force-pushed the refactor/extension-value branch from a9e4ab9 to e154e57 Compare April 24, 2018 16:28
@bajtos bajtos merged commit 5dc4724 into master Apr 24, 2018
@bajtos bajtos deleted the refactor/extension-value branch April 24, 2018 16:37
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.

6 participants