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: upgrade to openapi3-ts@0.11.0 #1265

Merged
merged 1 commit into from
Apr 20, 2018

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Apr 18, 2018

The release of openapi3-ts@0.11.0 closes the gap of OAS 3.0 spec.
This PR removes most of our own type declarations for the spec.

Checklist

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

*/
export interface ParameterObject extends ISpecificationExtension {
name: string;
in: ParameterLocation;
Copy link
Contributor

Choose a reason for hiding this comment

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

@raymondfeng The openapi3-ts package doesn't have ParameterLocation and ParameterStyle, so its ParameterObject doesn't verify the value of property in and style are valid

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.

Nice to know the new version patches SchemaObject to extend JSON types! 👍
I left a comment regarding ParameterLocation and ParameterStyle.
Adopting the community ParameterObject will lose the editor checking for the valid value of in and style, while validator can still catch invalid value at compile time. So I am fine with either:

  • adopt community ParameterObject: then ParameterLocation and ParameterStyle interface can be removed too.
  • keep the ParameterObject: if it causes incompatibility then no need to spend time on it.

@raymondfeng you can do whichever makes more sense.

Otherwise LGTM 🚢

@bajtos
Copy link
Member

bajtos commented Apr 19, 2018

Adopting the community ParameterObject will lose the editor checking for the valid value of in and style, while validator can still catch invalid value at compile time.

To me, it's important to allow the compiler to catch as many problems as possible - that's one of the reasons we switched from JS to TS.

Can we please preserve this feature for ParameterObject's in and style?

Ideally, we should contribute a fix to openapi3-ts.

For the short-term, I'd prefer to keep our own copy of type declarations needed to fix in and style.

Thoughts?

@@ -21,6 +22,10 @@ export type OpenApiSpec = OAS3.OpenAPIObject;
// tslint:disable-next-line:no-any
export type ExtensionValue = any;

export type SchemasObject = {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be called SchemaObject (singular Schema)? The old code has export interface SchemaObject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two types: SchemaObject (for schema definitions) and SchemasObject (for schemas of the spec).

@@ -21,6 +22,10 @@ export type OpenApiSpec = OAS3.OpenAPIObject;
// tslint:disable-next-line:no-any
export type ExtensionValue = any;
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT, ExtensionValue type is no longer used, could you please remove it too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still referenced in our code base to avoid any.

Copy link
Member

Choose a reason for hiding this comment

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

See #1271

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

Great work @raymondfeng !! Can we have a follow up task to contribute the remaining types to openapi3-ts so we don't have to maintain our own types package anymore.

@raymondfeng
Copy link
Contributor Author

See metadevpro/openapi3-ts#20

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

LGTM, but consider @bajtos's comment about preserving ParameterObject interface until your patch is landed in openapi3-ts.

The release of openapi3-ts@0.11.0 closes the gap of OAS 3.0 spec.
This PR removes most of our own type declarations for the spec.
@raymondfeng raymondfeng force-pushed the upgrade-to-openapi3-ts-0.10.0 branch from e7a1e64 to 1047700 Compare April 20, 2018 17:00
@raymondfeng raymondfeng changed the title feat: upgrade to openapi3-ts@0.10.0 feat: upgrade to openapi3-ts@0.11.0 Apr 20, 2018
@raymondfeng
Copy link
Contributor Author

Please openapi3-ts@0.11.0 is released with my patches.

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 🚢

@raymondfeng raymondfeng merged commit 1ed79c9 into master Apr 20, 2018
@raymondfeng raymondfeng deleted the upgrade-to-openapi3-ts-0.10.0 branch April 20, 2018 17:53
@bajtos
Copy link
Member

bajtos commented Apr 23, 2018

👏

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.

5 participants