fiber
fiber copied to clipboard
🔥 Feature!: Add Req and Res API
CURRENT STATUS
- [x] Response
- [x] Ctx.Request => Ctx.Req
- [x] Complete fiber.Request API
- [ ] Create Request Interface with a DefaultRequest implementation
- [x] Create Doc Page
- [x] Response API
- [x] Ctx.Response => Ctx.Res
- [x] Complete fiber.Response API
- [ ] Create Response Interface with a DefaultResponse implementation
- [x] Create Doc Page
Description
Split the existing Ctx API into two separate APIs for Requests and Responses. Having distinct APIs for Request and Response types will allow developers to more easily write self-documenting code by avoiding ambiguous methods such as Ctx.Cookie
and Ctx.Cookies
. Much of the existing Ctx API simply calls the corresponding Req or Res API. For example, Ctx.Get
calls Request.Get
and Ctx.Set
calls Response.Set
.
Because Express defines only res and req APIs with no context API, this change will make it easier to manage Fiber's adherence to Express's APIs. In addition, it will allow us to avoid needing to create certain one-off methods. For example, Ctx.GetRespHeader
can now become Ctx.Res().Get
Although the majority of the Ctx API is unchanged, Ctx.Request
and Ctx.Response
have been removed in favor of Ctx.Req
and Ctx.Res
. Now, instead of a raw fasthttp.Request
or fasthttp.Response
, we return fiber.Request
and fiber.Response
objects, which implement their respective APIs. As such, this is a breaking change for users who access Ctx.Request
and Ctx.Response
direct.
Inspired by Koa and fasthttp, both of which have individual request/response objects and a single context object whose methods are simply aliases for corresponding methods on the req/res APIs.
Deprecation:
-
Ctx.GetRespHeader
is replaced byCtx.Res().Get
Related to #2854
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
- [ ] Benchmarks: Describe any performance benchmarks and improvements related to the changes.
- [ ] Documentation Update: Detail the updates made to the documentation and links to the changed files.
- [ ] Changelog/What's New: Include a summary of the additions for the upcoming release notes.
- [ ] Migration Guide: If necessary, provide a guide or steps for users to migrate their existing code to accommodate these changes.
- [x] API Alignment with Express: Explain how the changes align with the Express API.
- [x] API Longevity: Discuss the steps taken to ensure that the new or updated APIs are consistent and not prone to breaking changes.
- [ ] Examples: Provide examples demonstrating the new features or changes in action.
Type of change
Please delete options that are not relevant.
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Enhancement (improvement to existing features and functionality)
- [ ] Documentation update (changes to documentation)
- [ ] Performance improvement (non-breaking change which improves efficiency)
- [ ] Code consistency (non-breaking change which improves code reliability and robustness)
Checklist
Before you submit your pull request, please make sure you meet these requirements:
- [ ] Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
- [ ] Conducted a self-review of the code and provided comments for complex or critical parts.
- [ ] Updated the documentation in the
/docs/
directory for Fiber's documentation. - [ ] Added or updated unit tests to validate the effectiveness of the changes or new features.
- [ ] Ensured that new and existing unit tests pass locally with the changes.
- [ ] Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
- [ ] Aimed for optimal performance with minimal allocations in the new code.
- [ ] Provided benchmarks for the new code to analyze and improve upon.
Walkthrough
The updates involve significant refactoring across various components of a web framework, focusing on altering method calls and accessing request and response objects through a unified context. This shift enhances code clarity and consistency, streamlining interactions with HTTP headers, session management, and logging functionalities.
Changes
Files | Summary of Changes |
---|---|
app_test.go , bind.go |
Updated method calls for handling HTTP headers and request/response objects. |
client/client_test.go , middleware/... , request.go , response.go |
Altered access to request and response data, and updated header management. |
ctx_interface.go , middleware/session/session.go , redirect.go |
Refactored to use updated context methods, added new fields, and deprecated old methods. |
router.go |
Modified routing logic to use updated request context properties. |
🐰✨ In the land of code where the bits align, A rabbit hopped through, refactoring fine. From request to response, through headers it weaved, A tapestry of changes, expertly conceived. Celebrate the craft, for improvements shine bright, In the glow of the code, under the moonlight. 🌙 🌟🐇
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
-
I pushed a fix in commit <commit_id>.
-
Generate unit testing code for this file.
-
Open a follow-up GitHub issue for this discussion.
-
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitai
in a new review comment at the desired location with your query. Examples:-
@coderabbitai generate unit testing code for this file.
-
@coderabbitai modularize this function.
-
- PR comments: Tag
@coderabbitai
in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:-
@coderabbitai generate interesting stats about this repository and render them as a table.
-
@coderabbitai show all the console.log statements in this repository.
-
@coderabbitai read src/utils.ts and generate unit testing code.
-
@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
-
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (invoked as PR comments)
-
@coderabbitai pause
to pause the reviews on a PR. -
@coderabbitai resume
to resume the paused reviews. -
@coderabbitai review
to trigger a review. This is useful when automatic reviews are disabled for the repository. -
@coderabbitai resolve
resolve all the CodeRabbit review comments. -
@coderabbitai help
to get help.
Additionally, you can add @coderabbitai ignore
anywhere in the PR description to prevent this PR from being reviewed.
CodeRabbit Configration File (.coderabbit.yaml
)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yaml
file to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request features, and share feedback.
- Follow us on X/Twitter for updates and announcements.
Codecov Report
Attention: Patch coverage is 95.37275%
with 36 lines
in your changes missing coverage. Please review.
Project coverage is 82.97%. Comparing base (
dbeca39
) to head (9ca819e
). Report is 71 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #2894 +/- ##
==========================================
+ Coverage 82.78% 82.97% +0.19%
==========================================
Files 115 117 +2
Lines 8409 8547 +138
==========================================
+ Hits 6961 7092 +131
- Misses 1109 1117 +8
+ Partials 339 338 -1
Flag | Coverage Δ | |
---|---|---|
unittests | 82.97% <95.37%> (+0.19%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@nickajacks1 Just so you are aware, the tests are very unstable/flaky right now in main
. This is because we no longer retry failed tests multiple times on the CI. This is being fixing in #2892
3/9: Moved some more methods from Ctx to Request. Rebased.
3/17: Rebase. Moved more trivial methods to Request.
Hi @gofiber/maintainers
Sorry this is taking so long. I've reached the first major milestone - the Request API is fully moved over to the Request type. I'd like to get a health check - does the overall direction still seem OK? If so, I suspect that moving the Response API over will be comparatively quick. None of the unit tests have been moved over. IMO that's quite a bit of work and shouldn't hold up v3 since it's not public facing and can be done at any time.
Even though Express has Locals as part of the res API, I feel like it makes more sense to keep it as part of Ctx since it doesn't inherently have anything to do with the response (or request for that matter). I'm going to leave in Ctx for now.
ok, sounds reasonable we can always adapt it later
Ok, the Response API is fully moved over.
Some remaining work: I didn't move Redirect to the Response API yet. The Redirect API has some dependencies on DefaultCtx that requires more thought. Moving Redirect to Res won't break the API in any way, so we can take our time thinking about this.
Next week I'll start on either the docs or the interface part.
I was thinking, is it actually that useful to make Request and Response interfaces if Ctx is already an interface and can thus be expanded instead of Request/Response? If we say that custom extensions should just go through Ctx, that might simplify both the API and implementation a bit. Thoughts @gofiber/maintainers ?
I was thinking, is it actually that useful to make Request and Response interfaces if Ctx is already an interface and can thus be expanded instead of Request/Response? If we say that custom extensions should just go through Ctx, that might simplify both the API and implementation a bit. Thoughts @gofiber/maintainers ?
I am interested in learning more about the approach of expanding the Ctx
interface for custom extensions, instead of introducing separate Request
and Response
interfaces. Could you please explain how this approach works and how it simplifies the API for developers?
In addition, can you help us by merging the changes added to the main branch, and updating your PR to use the new Bind() interface?
Thank you, great work so far, and I look forward to your reply!
I am interested in learning more about the approach of expanding the Ctx interface for custom extensions, instead of introducing separate Request and Response interfaces. Could you please explain how this approach works and how it simplifies the API for developers?
The two primary points I have in mind are:
- Any extension of behavior can already be handled by using a custom Ctx since the split implementation is at its core a kind of syntactic sugar. I have not been able to think of a use case for Req/Res interfaces that can't be handled with the Ctx interface.
- Since the
Ctx
andRequest
+Response
APIs have overlap, it may be less clear to developers which they should implement. If they want to updateSendString
to tee out some logs, should they do it by making a customCtx
or a customRespose
? I feel like having eitherCtx
orRequest
+Response
be interfaces may be more straightforward (and I lean towardCtx
). This is more what I meant when I implied that having all three ofCtx
,Request
, andResponse
be interfaces could make the API more complex.
In addition, can you help us by merging the changes added to the main branch, and updating your PR to use the new Bind() interface?
ye. will also probably be able to mark the PR ready for review soon.
Marking as ready for review. If it is decided to make Request and Response interfaces, I will update the PR. Regarding the docs, I think it might be better to get this in first and then write up the docs.
Finished docs in time, so I added them to this PR after all. I'd like some criticism on how I updated the docs. I took the approach of keeping everything on a single page and using subsections for Request and Response methods.
I'm not sure what the best way to document the whole "c.Send()
is the same as c.Res().Send()
" thing is. I basically just sprinkled both forms in the examples, but I wonder if that would be confusing to newcomers.
I put a big green Tip box at the top to highlight it:
I didn't individually list the Ctx methods that simply call a Req/Res method since I felt that would get overly verbose.
@nickajacks1 How about a table that shows both styles?
I am interested in learning more about the approach of expanding the Ctx interface for custom extensions, instead of introducing separate Request and Response interfaces. Could you please explain how this approach works and how it simplifies the API for developers?
The two primary points I have in mind are:
- Any extension of behavior can already be handled by using a custom Ctx since the split implementation is at its core a kind of syntactic sugar. I have not been able to think of a use case for Req/Res interfaces that can't be handled with the Ctx interface.
- Since the
Ctx
andRequest
+Response
APIs have overlap, it may be less clear to developers which they should implement. If they want to updateSendString
to tee out some logs, should they do it by making a customCtx
or a customRespose
? I feel like having eitherCtx
orRequest
+Response
be interfaces may be more straightforward (and I lean towardCtx
). This is more what I meant when I implied that having all three ofCtx
,Request
, andResponse
be interfaces could make the API more complex.
I'm not sure about making Req and Resp not an interface. We're still able to update Ctx methods but it won't affect response for example. And it may cause some confusions as well. For example, if i've updated SendString, it means i'll have different behaving resp and ctx methods
@efectn
For example, if i've updated SendString, it means i'll have different behaving resp and ctx methods
Yeah, and this would be a problem whether or not Response is an interface. If response is an interface and you only override Ctx.SendString, you still have different behavior between Response and Ctx. Gonna have to think about that one some more since we'll want to address that either way.
Changing the subject a little...If Ctx implements both Request and Response, there's a corner case trap developers might hit if both Request and Response share method signatures, such as Get:
printResponseContentType := func(r Response) {
fmt.Println(r.Get("Content-Type"))
}
printResponseContentType(c) // this prints the request's header, not the response's
The only ways I've thought of to get around this so far are to explicitly add methods to Request and Response that Ctx does not implement so that Ctx does not implement those interfaces OR enforce that Request and Response do not have overlapping methods (which eliminates one of the benefits of splitting the APIs). I'll brainstorm for clean ways to deal with it, but feel free to spitball ideas.
RE: overriding a method in Ctx but not Req/Res, resulting in differing behavior between e.g., c.Get
and c.Req().Get
, I fear the only real options to prevent/mitigate this that I could come up with may be to
- Establish a clear direction for how we want the usage of interfaces to go and provide a detailed guide for interface usage (which we should make anyway) that lists ways you can shoot yourself in the foot. This unfortunately still leaves the door open for people to not read the guide and still shoot themselves in the foot.
- Do not provide aliases in Ctx, which seems heavy handed and unpopular.
- Do not make Ctx an interface, which may not be an option at this stage.
I imagine the last two options are non-starters, so I will focus on the first. If the user overrides the Req or Res method instead of the Ctx method, everything is hunky dory. If they override Ctx methods that are also provided by Res or Res, we hit the case where c.Get and c.Req().Get do different things. This means that the undesirable case is only avoidable by making Req and Res interfaces. A relatively safe rule of thumb is probably that users should prefer to override at the Req/Res level if possible, falling back to Ctx if the situation requires it.
RE: overriding a method in Ctx but not Req/Res, resulting in differing behavior between e.g.,
c.Get
andc.Req().Get
, I fear the only real options to prevent/mitigate this that I could come up with may be to
- Establish a clear direction for how we want the usage of interfaces to go and provide a detailed guide for interface usage (which we should make anyway) that lists ways you can shoot yourself in the foot. This unfortunately still leaves the door open for people to not read the guide and still shoot themselves in the foot.
- Do not provide aliases in Ctx, which seems heavy handed and unpopular.
- Do not make Ctx an interface, which may not be an option at this stage.
I imagine the last two options are non-starters, so I will focus on the first. If the user overrides the Req or Res method instead of the Ctx method, everything is hunky dory. If they override Ctx methods that are also provided by Res or Res, we hit the case where c.Get and c.Req().Get do different things. This means that the undesirable case is only avoidable by making Req and Res interfaces. A relatively safe rule of thumb is probably that users should prefer to override at the Req/Res level if possible, falling back to Ctx if the situation requires it.
Maybe it's better if we don't make Req and Resp an interface and add a guide about customization