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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃搾 [v3 Maintenance]: Evaluate v3 Updates for Alignment with Express API and Document Findings #2715

Open
3 of 9 tasks
ReneWerner87 opened this issue Nov 7, 2023 · 7 comments

Comments

@ReneWerner87
Copy link
Member

ReneWerner87 commented Nov 7, 2023

Maintenance Task Description

The purpose of this issue is to review the recent changes in v3 against the Express API to identify and document any deviations or convergences. This will assist in ensuring that our API remains as consistent as possible with Express, making it easier for users to transition or use both frameworks interchangeably.

Tasks

  • List all recent changes in the v3 beta branch that pertain to the API surface.
  • Compare these changes with the current Express API documentation.
  • Identify any deviations from the Express API design and functionality.
  • Also, note any areas where the v3 API has become more aligned with Express.
  • Document these findings in a markdown file, categorizing them into deviations and alignments.
  • Discuss potential implications of these findings and suggest possible actions if necessary.

Expected Outcome

  • A detailed markdown document listing all the differences and similarities between the v3 beta API and the Express API.
  • Informed recommendations for maintaining API compatibility or consciously diverging where it benefits our users.

Checklist:

  • I have confirmed that this maintenance task is currently not being addressed.
  • I understand that this task will be evaluated by the maintainers and prioritized accordingly.
  • I am available to provide further information if needed.
@nickajacks1
Copy link
Member

One major deviation from Express is the combination of Request and Response into a single Ctx object. It might be worth considering splitting it into a Req and Res object. We could either pass these objects directly to the handler as Express does or access them through c.Request() and c.Response() (could shorten those to c.Req() and c.Res() for brevity).

Express:

app.get('/veggies/:name/color', function(req, res) {
    if (veggies.has(req.params.name)) {
        res.send(veggies.get(req.params.name).color)
    } else {
        res.sendStatus(404);
    }
});

Fiber:

app.Get("/veggies/:name/color", func(req fiber.Req, res fiber.Res) error {
	if veg, ok := veggies[req.Params("name")]; ok {
		return res.SendString(veg.Color)
	} else {
		return res.SendStatus(404)
	}
})

Some notes:

  • This would allow us to more easily keep track of alignment with Express in the future, as there will be a closer one-to-one mapping between portions of each's API. (Fiber's Req <=> Express's req)
  • It might be possible to keep Ctx around behind the scenes for more natural integration with fasthttp.RequestCtx
  • This reduces fiber's similarities with gin, echo, et al which use a single Context object passed to handlers. Interestingly, it increases similarity with net/http handlers which take a response writer and a request.
  • This appears nontrivial and may impact the goal of March 2024.

If we decide not to go this route, it would be nice to write down that decision somewhere for posterity (unless it's already written down and I missed it..)

@ReneWerner87
Copy link
Member Author

Interesting idea, think this would have been considered before, but decided against it because of the mass of parameters in other functions
Next also had to be considered as a parameter
It would be interesting to see how this would look in a real example if the complete fiber code was adapted

@ReneWerner87
Copy link
Member Author

There is also the fasthttp object or other general objects, where do we place them?

@nickajacks1
Copy link
Member

nickajacks1 commented Dec 2, 2023

I'll play around with my above suggestion in the coming weeks and try to come up with a summary of the impact. I will plan on making a separate issue or PR to track it.

@nickajacks1
Copy link
Member

It looks like Ctx.Append has different behavior than res.append in Express.

Ctx.Append adds to the existing header field, using comma separators. Example:

Vary: Thing1, Thing2

res.append will create a separate header field. Example:

Vary: Thing1
Vary: Thing2

By the way, fasthttp's equivalent, Header.Add, acts like res.append. If we align with Express here, we could simply call that function instead of doing the parsing we're doing today.

@sixcolors
Copy link
Member

sixcolors commented Mar 3, 2024

Current implementation seems to follow the RFC https://www.rfc-editor.org/rfc/rfc9110#field.vary for your example case.

@nickajacks1
Copy link
Member

Agree that if the RFC suggests to use comma separated values for Vary, Fiber should do that.
Now that I think about it, I wonder when you'd actually want a second copy of the same header with a different value...Both Express and Koa do that, but it does seem a bit strange. Vary, Accept, Link, and Range (and probably several others) all allow you to use lists using comma-separation.

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

No branches or pull requests

3 participants