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

Refactor lib to add support for OpenAPI v3 #140

Merged
merged 9 commits into from
May 23, 2019
Merged

Refactor lib to add support for OpenAPI v3 #140

merged 9 commits into from
May 23, 2019

Conversation

Mairu
Copy link
Collaborator

@Mairu Mairu commented Apr 27, 2019

Summary

This is more or less a rewrite of the library to allow support for swagger v2 and openapi v3, while trying to maintain as much backward compatibility as possible.

Changelog of this Pull Request:
BREAKING:

  • swagger/openapi specifications have to be defined in specs instead of the root of the config object (in favor of PR #51)
  • specifications for operations / methods have now to be in the nested key service.docs.operations instead of directly in service.docs
  • remove support of findQueryParameters option in favor of defaults with path support for nested structures
  • default model names of versioned apis are now separated with _ instead of space which is necessary because of #109
  • the overwriting of tags with a later registered service introduced with PR #69 is disabled by default and has to be activated with the overwriteTagSpec option #23
  • removed sequelize related utils getType, getFormat, property and definition

ADDED:

  • add option to ignore by paths
  • add options to specify which services to include to documentation by paths or tags
  • add appProperty option
  • add docsJsonPath option
  • add defaults option where generator functions or objects for customized defaults can be defined (extended version of PR #63)
  • add possibility to use paths as keys for setting only parts of the specification
  • add possibility to customize the refs used for different operations #116
  • allow disabling of specific service methods #63
  • add possibility to customize tag, tags and model for a service #24 #112
  • add support for OpenAPI v3 #137
  • only add tags if they have been used, see PR #125
  • add support for path parameters, see PR #70
  • add support for custom methods introduced with @feathersjs/express v4

FIXED:

  • do not use space in list ref #109

@Mairu Mairu changed the title [WIP] Refactor lib to add support for openapi v3 Refactor lib to add support for openapi v3 May 5, 2019
@Mairu Mairu changed the title Refactor lib to add support for openapi v3 Refactor lib to add support for OpenAPI v3 May 5, 2019
@Mairu
Copy link
Collaborator Author

Mairu commented May 5, 2019

@daffl Well this one got big. I hope this is no problem because you said a rewrite would be the best approach.

I added the changed stuff to the changelog. As I see you are using automated changelog generation, should I remove that and create one issue or multiple issues for every line that is not connected to a issue already?

Things I did not in this branch but that should be done:

  • npm dependencies debug and swagger-ui seems unused and should be removed
  • swagger-ui-dist could be a peerDependency (it is not used by default)
  • there are undocumented exported utils that are not used by the library internally: getType, getFormat, property and definition, possibilites:
    • remove them (they are sequelize related and could go into an own package)
    • document them and add OpenAPI v3 support (should be done in this pull request)
    • leave them as they are

@daffl
Copy link
Member

daffl commented May 5, 2019

This is great, thank you! I'll hopefully get to do a review next week. As for your questions

  • Yes, the Changelog will be generated automatically, the old one can be removed
  • Doesn't have to be super accurate. This PR here will be linked which should have all the details you'd need
  • There should be a migration section on the changes necessary to upgrade to the latest version
  • Yes, unused dependencies can be removed. I thought there was an option that allows hosting swagger-ui though (see https://github.com/feathersjs-ecosystem/feathers-swagger/blob/master/lib/index.js#L7)
  • Agreed, Sequelize specific stuff should not go in here and can be removed

Thanks again for doing this!

@Mairu
Copy link
Collaborator Author

Mairu commented May 5, 2019

The used dependency is swagger-ui-dist, which is used whenever the uiIndex option is not false. But swagger-ui is not used. As swagger-ui-dist is only used with set uiIndex option it could be a peerDependency but I am ok with keeping it a normal dependency.

Ok I will remove the unneeded dependencies, functions and changelog entries. I should be able to do this tomorrow or the day after.

Where should the migration section be documented?

Mairu added 2 commits May 6, 2019 08:58
…d utils, remove unused + update dependencies, add more tests
…stom method support, use all instead of __all
@Mairu
Copy link
Collaborator Author

Mairu commented May 18, 2019

@daffl Since you had not the time yet to review, I fixed a bug and added support for custom methods and path parameters. And renamed __all options I introduced to all to be more align with hooks, where it is also named all.
To be typescript compatible I had to nest the operation specific options in a separated key, which is an additional breaking change. All the breaking changes are documented in the readme, only the version number of the new version has to be inserted.

…ak), minor changes that occurred while writing tests
@Mairu
Copy link
Collaborator Author

Mairu commented May 23, 2019

Have added many tests with 100% coverage. Did some minor changes and introduced another breaking change, because with #69 tags will always be overwritten, which seems wrong to me.

I will do no more changes until requested.

@daffl
Copy link
Member

daffl commented May 23, 2019

Awesome. I will make a pre-release today. I also added you as a collaborator to the repository so you can make changes directly. Once it is out would you like to be able to publish new versions to npm as well?

@Mairu
Copy link
Collaborator Author

Mairu commented May 23, 2019

If you want I can also publish packages to npm.

@daffl daffl merged commit 9295fdc into feathersjs-ecosystem:master May 23, 2019
@daffl
Copy link
Member

daffl commented May 23, 2019

Released as v1.0.0, thank you for the great work!

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