blomqma / next-rest-framework

Type-safe, self-documenting APIs for Next.js
https://next-rest-framework.vercel.app
Other
155 stars 20 forks source link

Exclude sub paths #44

Closed almontasser closed 1 year ago

almontasser commented 1 year ago

This adds the option to exclude routes from OpenAPI schema generation, it's useful when there are some routes are written in the normal nextjs way. Otherwise they are getting called by this framework

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-rest-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2023 0:51am
next-rest-framework-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2023 0:51am
codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05 :tada:

Comparison is base (50f0fdc) 93.39% compared to head (e63e180) 93.44%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #44 +/- ## ========================================== + Coverage 93.39% 93.44% +0.05% ========================================== Files 10 10 Lines 1529 1541 +12 Branches 172 174 +2 ========================================== + Hits 1428 1440 +12 Misses 98 98 Partials 3 3 ``` | [Impacted Files](https://app.codecov.io/gh/blomqma/next-rest-framework/pull/44?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Markus+Blomqvist) | Coverage Δ | | |---|---|---| | [packages/next-rest-framework/src/utils/open-api.ts](https://app.codecov.io/gh/blomqma/next-rest-framework/pull/44?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Markus+Blomqvist#diff-cGFja2FnZXMvbmV4dC1yZXN0LWZyYW1ld29yay9zcmMvdXRpbHMvb3Blbi1hcGkudHM=) | `97.55% <100.00%> (+0.07%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

blomqma commented 1 year ago

I like the core idea of this optimization, although I have a few comments regarding it. The change you're proposing here only affects how the framework works in development and when running npx next-rest-framework generate. Currently the framework calls internally all of your API endpoints from your file system, regardless of whether they are defined with Next REST Framework or not. Like said, we only care about the endpoints that are defined with the framework and the extraneous endpoints might cause some redundant network calls, indeed.

The network calls that register the endpoints for the OpenAPI spec generation are done in a parallel manner, however. This means that in a vast majority of cases, the redundant network calls to the non-NRF endpoints cause such a small overhead to the overall OpenAPI spec generation that dropping those calls would be unnoticeable. In fact, all of the network calls are aborted if they do not get a response within 200ms and with the parallelization this happens very fast even with a large amount of endpoints.

I can see the use case though, where a developer might have e.g. dozens or hundreds of non-NRF endpoints and they would like to generate the OpenAPI spec only for a subset of endpoints defined with NRF. For such use case, I would propose an opt-in approach instead of excluding certain paths from the generation. So instead of the added excludeSubPaths config option, I think a better option would be to have a config option like matchApiRoutes, that would simply take an array of patterns (with wildcards) that would be used to find only the wanted API routes for the OpenAPI generation and the network calls would be done only for those endpoints. What do you think?