-
Notifications
You must be signed in to change notification settings - Fork 7
Chore: add OpenApi encoded #26
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
Conversation
src/serverless-openapi-typescript.ts
Outdated
| method.tags = [tagName]; | ||
| } | ||
|
|
||
| method.operationId = `${this.serverless.service.service}-${method.operationId}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field is also responsible for the url also - for example, https://docs.env0.com/reference/project-updatefunc
src/serverless-openapi-typescript.ts
Outdated
| encodeOpenApiToStandard(openApi) { | ||
| const INVALID_CHARACTERS_KEY = /<|>/g; | ||
| const INVALID_CHARACTERS_REF = /%3C|%3E/g; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I would add a comment about what we're doing here and why (how generics are generated with
>and how these aren't legal character's in openapi 3.1 spec (with link) %3Cand%3Edon't mean much - I wouldatob('>')to better capture the intent here, and also rename toINVALID_CHARACTERS_ENCODED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btoa - is to encode it to base 64.
Maybe you mean encodeURI
I can change it to something like that - But I prefer to keep it simple. I can add comment about it
new RegExp(`${encodeURI('<')}|${encodeURI('>')/g`)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/serverless-openapi-typescript.ts
Outdated
| ); | ||
|
|
||
| return mapValuesDeep(mapObject, (value, key) => | ||
| isString(value) && key === '$ref' && INVALID_CHARACTERS_REF.test(value) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think isString(value) is superfluous.
If key === '$ref', what else could it be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For safety - but we don't need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/serverless-openapi-typescript.ts
Outdated
| const mapObject = mapKeysDeep(openApi, (value, key) => | ||
| INVALID_CHARACTERS_KEY.test(key) ? key.replace(INVALID_CHARACTERS_KEY, '_') : key | ||
| ); | ||
|
|
||
| return mapValuesDeep(mapObject, (value, key) => | ||
| isString(value) && key === '$ref' && INVALID_CHARACTERS_REF.test(value) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matter of personal of style so your call - I think the interim value of mapObject does nothing but make this method a little more verbose than it needs to be.
Of course it makes sense to avoid nested functions like f(g(x)).
Deepdash, like lodash does offer a way to chain functions which make it easier to follow the functional path:
_(openApi)
.mapKeysDeep(stubGenerics)
.mapValuesDeep(stubBase64EncodedGenerics)
.value();There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see they offer this chain - they offer to put the functions on the lodash object _ but I think not in the chain (more complex to do it) But I will try. I searched after that in the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like how they mix it with lodash and it will not work with the chain also we have typescript problem.
// or cherry-pick method you only need and mix it into lodash
require('deepdash/addFilterDeep')(_);
// or cherry-pick method separately if you don't want to mutate Lodash instance
const filterDeep = require('deepdash/getFilterDeep')(_);
I tried to do something with flow - but it just complex it and increase the code
| { | ||
| "name": "serverless-openapi-typescript", | ||
| "version": "1.0.10", | ||
| "version": "2.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed major version because we broke the API.
operationId value will be different after this version
Readme API docs - enforce open API standard that
<,>are invalid characters and uniqe operationId.ts-json-schema-generatorto generate the schema - howeverts-json-schema-generatorgenerate<,>when your types include generic types.@conqa/serverless-openapi-documentationto enrich data. openapi-documentation uses the function name for the operation id.Solutions:
<,>to_Breaking changes
operationId was change