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

fix: rename definitions to properties #50

Merged
merged 5 commits into from
Jul 31, 2022
Merged

Conversation

qwelias
Copy link
Collaborator

@qwelias qwelias commented Jul 26, 2022

@qwelias
Copy link
Collaborator Author

qwelias commented Jul 28, 2022

hi @Coobaha can you have a look?

@Coobaha
Copy link
Owner

Coobaha commented Jul 28, 2022

@qwelias oh sorry, missed it - thanks for the ping :) will check soon!

@Coobaha
Copy link
Owner

Coobaha commented Jul 28, 2022

@qwelias can you please ensure that tests are passing? 🙇

@qwelias
Copy link
Collaborator Author

qwelias commented Jul 28, 2022

yep, my bad, totally forgot about them

@qwelias
Copy link
Collaborator Author

qwelias commented Jul 28, 2022

There's still a problem with ajv validation I think. It does not allow namespace keyword in error strict mode, not sure what to do about it.

@qwelias qwelias changed the title fix: rename definitions to namespace fix: rename definitions to properties Jul 28, 2022
@qwelias
Copy link
Collaborator Author

qwelias commented Jul 28, 2022

@Coobaha should be good to go now

tho realistically it's a breaking change in case anyone used definitions in genereted json

@Coobaha
Copy link
Owner

Coobaha commented Jul 30, 2022

@qwelias Thanks for the contribution. Trying to understand the issue and how this PR fixes it. So when typed-fastify is used together with fastify-swagger it mutates internal json schema that is shared with fastify validations? So root issue is that something between this two plugins are incompatible or?

On the first look it seems to create new object that it mutates by deletion here https://github.com/fastify/fastify-swagger/blob/47ab4b5caca673b444932f7a45f63a1af15e4ca8/lib/spec/swagger/index.js#L22-L25

    swaggerObject.definitions = prepareSwaggerDefinitions({
      ...swaggerObject.definitions,
      ...(ref.definitions().definitions)
    }, ref)

Please correct me if I am wrong or even better pleasee provide stackblitz example with repro.

@qwelias
Copy link
Collaborator Author

qwelias commented Jul 30, 2022

@Coobaha https://github.com/qwelias/typed-fastify-test
there's only two commits, reset between them, compare http://127.0.0.1:3000/openapi/json responses, click around in swagger interface

@Coobaha Coobaha merged commit edda501 into Coobaha:main Jul 31, 2022
@Coobaha
Copy link
Owner

Coobaha commented Jul 31, 2022

@qwelias while i understand that this fixes an issue, to me it seems that this is something that should be fixed in https://github.com/fastify/fastify-swagger many of json schema examples are using definitions, not $defs. Will explore more also in this plugin

@qwelias
Copy link
Collaborator Author

qwelias commented Jul 31, 2022

I believe it depends on how you fastify.addSchema those definitions. In your case you do just one call with whole schemas object instead of adding each definition separately.

I thought about fixing it by adding each definition, but it may introduce conflicts between multiple generated schemas since you can export same type names from different .ts files

@Coobaha
Copy link
Owner

Coobaha commented Jul 31, 2022

@qwelias i've published latest version (including changes from this PR) https://www.npmjs.com/package/@coobaha/typed-fastify/v/1.0.0-rc.1

Please let me know if you will have any issues :)

@qwelias qwelias deleted the fix-swagger branch August 1, 2022 12:39
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.

None yet

2 participants