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

DRAFT: 2.x renovation - multiple APIs #14

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rela589n
Copy link
Contributor

Hi, @fre5h

I'm interested in upgrading this package by adding new features that large projects could need.

A first key enhancement current PR focuses on is providing multiple APIs tailored to the needs of various teams.

Some projects could necessitate more than just one frontend API. For example, there might be a demand for entirely separate API specifications for ERP or CRM teams, each with a unique set of endpoints and a distinct authentication system. Having a single api documentation for few parties exposes too much excessive details to them and brings unnecessary complexity.

As a result, it's practical to offer the capability to create fully independent API specifications designed for different people.

Currently I've updated README.md to show my vision on how this configuration could look like. Please, let me know what you think about it and whether can I proceed with the implementation?

@rela589n rela589n requested a review from fre5h as a code owner March 23, 2024 10:22
@rela589n rela589n changed the title 2.x renovation - multiple APIs DRAFT: 2.x renovation - multiple APIs Mar 23, 2024
Copy link

codecov bot commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.80%. Comparing base (cd6f200) to head (c17ef81).

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #14       +/-   ##
===========================================
+ Coverage        0   91.80%   +91.80%     
- Complexity      0       20       +20     
===========================================
  Files           0        2        +2     
  Lines           0       61       +61     
===========================================
+ Hits            0       56       +56     
- Misses          0        5        +5     
Flag Coverage Δ
unittests 91.80% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rela589n rela589n marked this pull request as draft March 23, 2024 10:29
@@ -47,7 +47,7 @@
"config": {
"sort-packages": true,
"allow-plugins": {
"symfony/flex": true
"symfony/flex": false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why have you disabled symfony flex?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is not an ultimate symfony project that needs recipes executed. Currently, running composer install executes all the recipes right in the SwaggerBundle's root. I don't think it should be the case, since it only brings the disturbing experience to anyone who would like to contribute.

```

Imagine you have a Swagger spec like this:
Imagine you are to have a Swagger spec like this:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous variant is correct. this one sounds wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The meaning is slightly different. What is meant here could be rephrased in following way:

Imagine that you need to have following Swagger specification:

If it's ok with you, I'll use this description, if not - previous variant will be kept

```yaml
swagger:
config_folder: '%kernel.project_dir%/docs/api/'
apis:
frontend:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be an option if you have multiply api docs. but previous way should also be supported. because backwards compatibility will be broken.
so in the result it should support old and new implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are 100% right that this is the BC break. Although, as for me, maintenance of the current config will be a real headache. Taking into account that migration to the new approach is really simple:

swagger:
-  config_folder: '%kernel.project_dir%/docs/api/'
+  apis:
+    frontend:
+      config_folder: '%kernel.project_dir%/docs/api/'
+      doc_folder: '%kernel.project_dir%/public/api/'

I would suggest releasing a new major version that breaks this compatibility and writing an UPGRADE guide to point out the desired changes.

In any case, if you wish, I could implement the feature w/o BC support and you could add this support afterwards, if needed.

@fre5h fre5h self-assigned this Mar 24, 2024
@fre5h fre5h added the enhancement New feature or request label Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants