fiber icon indicating copy to clipboard operation
fiber copied to clipboard

🔥 Feature!: Add Req and Res API

Open nickajacks1 opened this issue 4 months ago • 19 comments

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 by Ctx.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.

nickajacks1 avatar Mar 03 '24 23:03 nickajacks1

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?

Share
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.

coderabbitai[bot] avatar Mar 03 '24 23:03 coderabbitai[bot]

Codecov Report

Attention: Patch coverage is 95.37275% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 82.97%. Comparing base (dbeca39) to head (9ca819e).

Files Patch % Lines
request.go 95.15% 13 Missing and 4 partials :warning:
response.go 94.14% 11 Missing and 4 partials :warning:
ctx.go 98.41% 1 Missing :warning:
ctx_interface.go 96.00% 1 Missing :warning:
middleware/cache/cache.go 90.90% 1 Missing :warning:
middleware/filesystem/filesystem.go 87.50% 1 Missing :warning:
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.

codecov[bot] avatar Mar 05 '24 03:03 codecov[bot]

@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

gaby avatar Mar 05 '24 03:03 gaby

3/9: Moved some more methods from Ctx to Request. Rebased.

nickajacks1 avatar Mar 10 '24 00:03 nickajacks1

3/17: Rebase. Moved more trivial methods to Request.

nickajacks1 avatar Mar 18 '24 01:03 nickajacks1

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.

nickajacks1 avatar Apr 15 '24 00:04 nickajacks1

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.

nickajacks1 avatar Apr 21 '24 17:04 nickajacks1

ok, sounds reasonable we can always adapt it later

ReneWerner87 avatar Apr 21 '24 19:04 ReneWerner87

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.

nickajacks1 avatar Apr 21 '24 21:04 nickajacks1

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 ?

nickajacks1 avatar May 12 '24 17:05 nickajacks1

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!

sixcolors avatar May 13 '24 00:05 sixcolors

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:

  1. 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.
  2. Since the Ctx and Request+Response APIs have overlap, it may be less clear to developers which they should implement. If they want to update SendString to tee out some logs, should they do it by making a custom Ctx or a custom Respose? I feel like having either Ctx or Request+Response be interfaces may be more straightforward (and I lean toward Ctx). This is more what I meant when I implied that having all three of Ctx, Request, and Response 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.

nickajacks1 avatar May 13 '24 01:05 nickajacks1

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.

nickajacks1 avatar May 13 '24 01:05 nickajacks1

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: chrome_8rQ4swzyeG I didn't individually list the Ctx methods that simply call a Req/Res method since I felt that would get overly verbose.

nickajacks1 avatar May 19 '24 23:05 nickajacks1

@nickajacks1 How about a table that shows both styles?

gaby avatar May 19 '24 23:05 gaby

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:

  1. 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.
  2. Since the Ctx and Request+Response APIs have overlap, it may be less clear to developers which they should implement. If they want to update SendString to tee out some logs, should they do it by making a custom Ctx or a custom Respose? I feel like having either Ctx or Request+Response be interfaces may be more straightforward (and I lean toward Ctx). This is more what I meant when I implied that having all three of Ctx, Request, and Response 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 avatar May 24 '24 21:05 efectn

@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.

nickajacks1 avatar May 25 '24 20:05 nickajacks1