ADB: modified OpenAPI @extension decorator to accept path array for key#3
ADB: modified OpenAPI @extension decorator to accept path array for key#3
Conversation
| "vitest": "^3.1.2", | ||
| "yaml": "~2.7.0" | ||
| "yaml": "~2.7.0", | ||
| "es-toolkit": "~1.36.0" |
There was a problem hiding this comment.
I would prefer if we didn't need to add another dependency just for this small use case.
It should also be added to the packages/openapi3/package.json here this is just for our workspace dev dependency which have no effect in published packages
There was a problem hiding this comment.
Yes, will replace this with a function once we have the other parts sorted out.
| { | ||
| "path": "library-ts/package-lock.json", | ||
| "destination": "package-lock.json" | ||
| }, |
There was a problem hiding this comment.
that should be reverted, if you did that just for testing
There was a problem hiding this comment.
No, the pnpm build created it. Can remove.
| * In OpenAPI only unknown properties starting with `x-` are allowed. | ||
| * Type for extension keys, allowing both single string keys and arrays of strings. | ||
| */ | ||
| export type ExtensionKey = `x-${string}`; |
There was a problem hiding this comment.
I think I would keep ExtensionKey to be this format as this does define accurately what you are allowed to do in openapi but instead just make it that the map of extension key to value is just a Map<string, unknown>
There was a problem hiding this comment.
As noted in my other comment, this tends to suggest that we shouldn’t be using the @extension decorator to add attributes deep within the (non-extension) nested structure of the OpenAPI op mapping.
| */ | ||
| export function getExtensions(program: Program, entity: Type): ReadonlyMap<ExtensionKey, any> { | ||
| return program.stateMap(openApiExtensionKey).get(entity) ?? new Map<ExtensionKey, any>(); | ||
| export function getExtensions(program: Program, entity: Type): Array<ExtensionRecord> { |
There was a problem hiding this comment.
I think changing that will be quite a breaking change for anyone using the API.
was there an issue with saving the new data into program.stateMap(openApiDeepExtensionKey); or something like that and exposing a separate function that return those.
There was a problem hiding this comment.
I preferred the approach of mapping string to a singleton array in the statemap: avoids semantic messiness. That said, if we have third-party apps calling this function then I agree that it makes backwards compatibility a problem. Have you discussed yet with your colleagues? If we are to distinguish the “deep” extensions then I’d prefer to have a different decorator to avoid that messiness.
Note that the need to bypass the “x-“ constraint on OpenAPI extensions also suggests we might consider a different decorator.
| } | ||
|
|
||
| function isExtensionKey(key: string): key is ExtensionKey { | ||
| function isExtensionKey(key: string): boolean { |
There was a problem hiding this comment.
inline with the other comments with ExtensionKey I think that should be reverted too
There was a problem hiding this comment.
Probably. Will resolve according to the resolution of questions posed above.
2 participants
No description provided.