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(anchors): applied a fix for anchors living in seperate files with… #256

Merged

Conversation

goldsziggy
Copy link
Contributor

… the same keyname

this is done by changing the property merge strategy to performing a deepMerge ignoring null values

fix #255

… the same keyname

this is done by changing the property merge strategy to performing a deepMerge ignoring null values

fix Surnet#255
@kalinchernev
Copy link
Contributor

I haven't spent the time to reproduce locally, but seeing that all tests pass with such a little surface of change, I'm impressed :) Good job! The only thing I'd ask for is to have the test and the example in the existing example folder and not have 2 separate examples for this feature. In future, I want to use these example folders as a sort of a tutorial/FAQ section and i'll surely merge them at some point. If you could do it, then it'll be a perfect change with no further changes to do in future :)

Regarding a few points from the issue thread

After reading through the code I found by adding the x- to the root-key we bypass them being placed in the path (not sure if intended purpose), but that duplication than caused this bug.

It's an intended behavior which you most probably have figured already https://github.com/Surnet/swagger-jsdoc/blob/master/src/specification.js#L118-L127 It's there because i'm not using openapi specs in my full-time job and i am unable to follow along all the vendor-specific prefixes on the market. There was a case for the x-webhooks which is handled in a special way and feel free to add x-template in a similar way. I don't plan to change the flow of extensions any time soon, but if you add a specific case for this template approach, which i like and support, it'll be a red flag for any modifications in the future and will solidify the use case.

Once again: many thanks for adding this pull request and making the library even more effective!

@goldsziggy
Copy link
Contributor Author

@kalinchernev thanks for looking at what is there, I did just push a new commit consolidating the examples and expanding the existing test to handle the use-case I describe.

as for implementation I am indifferent if the approach the you end up with is specifically looking for x-template or doing the deep merge ignoring nulls.

In terms of unexpected impact, the x-template check would be the safest as there is no telling everyone's use-case and if the null removal would have any affect.

But in the mean time I did update the test-cases. So if you have a preferred approach let me know I can update the PR.

Thanks for the taking the time!

@kalinchernev kalinchernev merged commit 6f0d934 into Surnet:master Mar 7, 2021
@kalinchernev
Copy link
Contributor

@goldsziggy many thanks for your valuable contribution! It's been released in 6.0.9! 🚀

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.

Multiple Anchors in multiple files create nulls in the swagger result
3 participants