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

[Proposal v3] 🔥feat!: add Req and Res objects to mirror Express #2764

Closed
wants to merge 1 commit into from

Conversation

nickajacks1
Copy link
Member

@nickajacks1 nickajacks1 commented Dec 15, 2023

Description

DRAFT/Proposal:

Align with Express's idiomatic req/res APIs (v4) by adding the req and res objects. The intent is to make the translation of Express's APIs to Fiber's APIs more natural by separating request and response based methods by design. Not only will this make it easier to move from Express to Fiber, but it will also make it easier to internally evaluate API alignment.

The core change that must be made to satisfy the goal of this PR is to add Req and Res structs/interfaces that are accessible in handles and middlewares.

This is a very early incremental step to get feedback on what direction we should take with this, and more importantly, whether we want to do this at all.

Possible Approaches

There are two main approaches that could be taken:
1. Keep Ctx as a concept, but modify handler and middleware signatures to take a Res and Req parameter instead of a Ctx.

  • Pros:
    • Usage of Fiber handlers becomes closer to that of Express handlers. It becomes easier for Express developers to adopt Fiber.
    • net/http users will be familiar with the request+response parameter pattern (note that this wouldn't add additional net/http compatibility however)
  • Cons:
    • High impact on existing API (handler and middleware function signatures)
    • Potentially more difficult to migrate to Fiber for Gin/Echo/Iris users who are used to the Ctx pattern.

Example:

	app.Get("/stuff", func(req fiber.Req, res fiber.Res) error {
		id := req.Params("id")
		res.SendString("Your id: " + id)
	})

2. Keep Ctx as the parameter for handlers and middlewares and convert the Response() and Request() methods to return Res and Req objects in which the Express res and req APIs are implemented.

  • Pros:
    • Handler and middleware function signature need not change
    • I plan on implementing this first as an incremental step even if we go with approach 1.
  • Cons:
    • Not quite as obvious/natural for people used to Express handlers
    • A little more verbose ( c.Res() vs res )

Example:

	app.Get("/stuff", func(c fiber.Ctx) error {
		id := c.Req().Params("id")
		c.Res().SendString("Your id: " + id)
	})

In my own opinion, I think function signatures that match what Express has is a big deal for making Fiber intuitive for that specific audience, so I lean toward option 1.

Important Considerations

  • The underlying fasthttp object(s) and fiber implementation details must still be accessible. I believe this is achievable in a clean way for both approaches 1 and 2.
  • The target of March 2024 for v3 could be impacted as the scope of the change is not fully understood and I am not 100% sure what my availability will be for working on this.

TODO

  • Request
  • Response
    • res.app
    • res.headersSent
      • Doc only; in fasthttp, data is sent after the handler returns
    • res.locals
    • res.append()
    • res.attachment()
    • res.cookie()
    • res.clearCookie()
    • res.download()
    • res.end()
    • res.format()
    • res.get()
    • res.json()
    • res.jsonp()
    • res.links()
    • res.location()
    • res.redirect()
    • res.render()
    • res.send()
    • res.sendFile()
    • res.sendStatus()
    • res.set()
    • res.status()
    • res.type()
    • res.vary()

Changes Introduced

  • 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.
  • API Alignment with Express: Explain how the changes align with the Express API.
  • 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

  • New feature (BREAKING CHANGE which adds functionality)
  • Enhancement (improvement to existing features and functionality)
  • Documentation update (changes to documentation)
  • 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.

Add a minimal Res() API as an incremental experimental step toward the
idiomatic Express req/res API. This first change adds support for the
following portions of the res API:
* res.app
* res.append
* res.get
* res.set

The underlying fasthttp struct can be accessed using res.FastHTTP().

See https://expressjs.com/en/api.html#res
@nickajacks1 nickajacks1 changed the title feat!: add Res object to Ctx to mirror Express [Proposal v3] 🔥feat!: add Res object to Ctx to mirror Express Dec 15, 2023
@ReneWerner87 ReneWerner87 added this to the v3 milestone Dec 16, 2023
@nickajacks1 nickajacks1 changed the title [Proposal v3] 🔥feat!: add Res object to Ctx to mirror Express [Proposal v3] 🔥feat!: add Req and Res objects to mirror Express Dec 16, 2023
@ReneWerner87
Copy link
Member

prefer option 2 right now

as we have to accommodate functionalities that do not belong in either request or response
such as the next function https://docs.gofiber.io/api/ctx#next, app access https://docs.gofiber.io/api/ctx#app, context https://docs.gofiber.io/api/ctx#context, userContext https://docs.gofiber.io/api/ctx#setusercontext, clientHelloInfo, https://docs.gofiber.io/api/ctx#route etc.
and the handler interface changes quite a lot

@nickajacks1
Copy link
Member Author

prefer option 2 right now

as we have to accommodate functionalities that do not belong in either request or response such as the next function https://docs.gofiber.io/api/ctx#next, app access https://docs.gofiber.io/api/ctx#app, context https://docs.gofiber.io/api/ctx#context, userContext https://docs.gofiber.io/api/ctx#setusercontext, clientHelloInfo, https://docs.gofiber.io/api/ctx#route etc. and the handler interface changes quite a lot

Next:

yes, next would need to be added to the function signature to mirror Express.

App access

res and req both hold references to the underlying app https://expressjs.com/en/api.html#req.app

Route

req has a reference to the route object https://expressjs.com/en/api.html#req.route

userContext

yes, there are some features that we'd need to make sure are still accessible, for example by moving to req/res or making the context accessible from req/res.

Handler interface changes quite a lot:

I agree, this is probably the biggest downside in my opinion and is the primary reason I'm not strongly advocating for option 1. In fact, since option 2 is really a subset of option 1, it's probably prudent to limit this PR to option 2 anyway (if the idea is approved at all).

@etroynov
Copy link

@ReneWerner87

Hi,

If you will take second options it will be looks like koa:
https://github.com/koajs/koa

Just for info can you take a look how it was implemented in embed bun server:
https://bun.sh/docs/api/http

@ReneWerner87
Copy link
Member

I also like option 1, but we need good solutions for the mentioned methods

@nickajacks1
Copy link
Member Author

I think res.headersSent would always return false because of the way fasthttp only sends data over the wire after the handler returns. It would be at best useless and at worst misleading to include in Fiber. I'll note that in the documentation somewhere.

@nickajacks1
Copy link
Member Author

Ok, I can already tell this PR is going to get way too big to review very quickly.
If we go forward with this, perhaps it should be done on a separate branch over multiple PRs?

@ReneWerner87
Copy link
Member

Discord conversation

Ctx vs Req & Res API Change Discussion

nick

#2764
tl;dr of the above ^ PR: migrate the API away from having a singular Ctx object in handlers and split all/most of the functionality between Request and Response objects in order to be closer to Express.

I was wondering about how to best approach this proposal in terms of gauging how the community would receive it. My primary concern is about whether it would be considered a positive or needless change by the userbase. It will have a nontrivial effect on how it feels to use Fiber, so I worry about polarized reactions. It would be easy for me point to the goal of feeling like Express in order to justify the change, but I realize that doesn't automatically justify it.

I guess my concerns can boil down to one main question: how do I know if this change is a good idea?

If it's preferred to discuss directly on GitHub that's fine too, just wanted to limit the noise.

René

good question whether it is worth it
switching from express to fiber would definitely be easier and the individual functions would be better separated
there have already been some problems because people didn't know if this functionality belongs to request or response and we could also improve some functions that already have request/response in their name

on the other hand it's a lot of rebuilding work and we don't know if you always need it in the request function, then it would be an unused variable
then we still have the challenge to put the functions which are not assignable to request and response (context, next .....) somewhere

what are your opinions ?

uwunicode

it's a lot more common for frameworks (primarily javascript frameworks) to have separate req and res arguments (and maybe a next argument too). i think right now fiber is closer to koa than express in terms of that.

Abi

i think there's a lot of potential ambiguity and confusion in the current system (eg. with setting/retrieving cookies is the most glaring one I can think of - I still get them confused and ive been using fiber for 3 years) but also it increases the friction to upgrade major versions - but I dont think that would outweigh the benefits of splitting the objects, it'd just mean it needed to be done really well and have good migration docs/tools

uwunicode

still, i think this change would be too big and lots of tutorials will be deprecated because of it, not to mention the refactoring people would need to do to upgrade fiber versions
Gaby — heute um 13:55 Uhr
I like the way it is right now. Reworking all the core, middlewares, contrib, 3rd party middlewares plus all the users will have to change their code.

It only really benefits new users.
^ Yup what @uwunicode said

uwunicode

koa.js has a context attribute and an optional next attribute, that pattern works well too i think

Abi

something else could be having the two different signatures and supporting both but preferring the new form? I can see this becoming needlessly complex though

Степан

This is too radical change even for a major version. Especially since we don’t have an answer where to put functions like Next, Locals, etc.

uwunicode

express has a separate optional next argument in the handler functions and locals are stored within req
we could do varargs and reflection to support both patterns, but that might slow down the framework as a whole and i don't think it would be worth it

Abi

yeah probably not

uwunicode

alternatively, we could have separate methods for the req/res pattern, but that will add way too much complexity

Gaby

I can see people avoiding migrating to v3 if their application is too big.

René

you mean something like this
ctx.Request().Get("...")
ctx.Response().Set("...")

uwunicode

nah, just an alternative way to declare the routes
and instead of ctx those routes would receive req/res
again, i think it's too much complexity
and might cause confusion in general

René

yes fair points

Abi

further consideration on my part makes me think that the effort it'd take is better spent elsewhere tbh

uwunicode

yup
if it ain't broke don't fix it
we've had this pattern for many years now, suddenly changing it would basically mean making a new framework

René

ok i.e. we leave it as it is for now, due to the size of the change, until we know how we can implement it cleanly so that it can be easily migrated or the old methods still work

we have decided against the adaptation for the time being, as this would be too big a conversion and it is not clear how we can guarantee the users an easy migration

@kinbiko
Copy link
Contributor

kinbiko commented Jan 2, 2024

Please forgive the after-the-fact comment, but I'd like to add another (minor) argument that I don't see mentioned yet, should this come up again in the future.

A common pattern for implementing HTTP endpoints is to create request and response body types within the function body. Typically these types, and variables based on the types, are named req/request / res/response.
Changing the signature is likely to pollute the scope with identifiers developers would want to use to type their request- and response bodies.

Of course, users can name their variables something else -- but "naming things" is extra cognitive overhead.

@nickajacks1
Copy link
Member Author

Thank you for the very thorough discussion!

I do think it's worth thinking about separating or clarifying the request-specific vs response-specific APIs as the ambiguity has real effects on usability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants