Skip to content

feat(openapi-v3): add OAS3 visibility decorator#6414

Merged
raymondfeng merged 1 commit into
loopbackio:masterfrom
achrinzafork:feat/adds-oas-visibility-decorator
Oct 6, 2020
Merged

feat(openapi-v3): add OAS3 visibility decorator#6414
raymondfeng merged 1 commit into
loopbackio:masterfrom
achrinzafork:feat/adds-oas-visibility-decorator

Conversation

@achrinza

Copy link
Copy Markdown
Member

closes #6392

Signed-off-by: Rifa Achrinza 25147899+achrinza@users.noreply.github.com

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • 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

👉 Check out how to submit a PR 👈

Comment thread packages/openapi-v3/src/types.ts Outdated
tags: string[];
}

export type VisibilityDecoratorMetadata = 'documented' | 'undocumented';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

enum or union type, which one is better?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Personally, I am fine with the union type.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

enum may be useful for pure-JavaScript experiences, but I'm ok with either.

@achrinza achrinza Sep 24, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Changed to enum to be in line with existing packages (e.g. repository, authorization, context`)

Will need to fix the errors. Errors fixed.

@bajtos bajtos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is awesome @achrinza! I like that you included detailed documentation, extensive test coverage and the code is closely following our coding style 👏🏻

I have few comments to consider, PTAL 👇🏻

BTW are there any places where we could replace explicit x-visibility: undocumented with @oas.visibility() decorator instead? It would be great to update https://loopback.io/doc/en/lb4/creating-components-rest-api.html#undocumented-endpoints to show the new approach.

Comment thread docs/site/decorators/Decorators_openapi.md
Comment thread packages/openapi-v3/src/controller-spec.ts Outdated
Comment thread packages/openapi-v3/src/types.ts Outdated
tags: string[];
}

export type VisibilityDecoratorMetadata = 'documented' | 'undocumented';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Personally, I am fine with the union type.

Comment thread packages/openapi-v3/src/types.ts Outdated
Comment thread packages/openapi-v3/src/types.ts Outdated
@achrinza

achrinza commented Sep 26, 2020

Copy link
Copy Markdown
Member Author

I've modified the behaviour to explicitly keep the value of x-visibility: 'documented' when using @oas.visibility(OperationVisibility.DOCUMENTED) - the same behaviour when setting x-visibility directly.

If need be, this should be stripped away by RoutingTable instead.

@bajtos bajtos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks pretty good 👏🏻

I have two minor comments to consider 👇🏻

Comment thread docs/site/extending/rest-api.md Outdated
Comment thread docs/site/extending/rest-api.md Outdated
closes loopbackio#6392

Co-authored-by: Miroslav Bajtoš <mbajtoss@gmail.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
@achrinza
achrinza force-pushed the feat/adds-oas-visibility-decorator branch from abd55fb to aa0a15f Compare September 29, 2020 09:10
@achrinza
achrinza requested a review from raymondfeng October 1, 2020 13:59
@raymondfeng
raymondfeng merged commit c388cbf into loopbackio:master Oct 6, 2020
@achrinza
achrinza deleted the feat/adds-oas-visibility-decorator branch October 6, 2020 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a new decorator @visibility('undocumented')

3 participants