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

🔥 v3: update Ctx.Format to match Express's res.format #2766

Merged
merged 4 commits into from
Jan 4, 2024

Conversation

nickajacks1
Copy link
Member

@nickajacks1 nickajacks1 commented Dec 17, 2023

Description

While the existing Ctx.Format provides a concise convenience method for basic content negotiation on simple structures, res.format allows developers to set their own custom handlers for each content type.

Docs: moved the existing Ctx.Format docs to a section called AutoFormat and added docs for the new Ctx.Format.

I'm taking suggestions for better names than Fmt for the mediaType-handler struct...was just worried about making it overly long since people will need to type it over and over.

Benchmarks

Performs ever so slightly better than AutoFormat (formerly known as Format)

Benchmark_Ctx_Format/with_arg_allocation-8               2578868               464.5 ns/op            96 B/op          1 allocs/op
Benchmark_Ctx_Format/with_arg_allocation-8               2577880               466.9 ns/op            96 B/op          1 allocs/op
Benchmark_Ctx_Format/with_arg_allocation-8               2596575               462.0 ns/op            96 B/op          1 allocs/op
Benchmark_Ctx_Format/with_arg_allocation-8               2564602               460.6 ns/op            96 B/op          1 allocs/op
Benchmark_Ctx_Format/pre-allocated_args-8                2983128               407.3 ns/op             0 B/op          0 allocs/op
Benchmark_Ctx_Format/pre-allocated_args-8                3031767               400.3 ns/op             0 B/op          0 allocs/op
Benchmark_Ctx_Format/pre-allocated_args-8                3045684               397.7 ns/op             0 B/op          0 allocs/op
Benchmark_Ctx_Format/pre-allocated_args-8                2986521               407.1 ns/op             0 B/op          0 allocs/op
Benchmark_Ctx_Format/text/plain-8                       12827778                92.41 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_Format/text/plain-8                       13453144                93.66 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_Format/text/plain-8                       13534003                86.67 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_Format/text/plain-8                       13168622                88.81 ns/op            0 B/op          0 allocs/op
Benchmark_Ctx_Format/json-8                              9903925               128.8 ns/op             0 B/op          0 allocs/op
Benchmark_Ctx_Format/json-8                              9780688               124.1 ns/op             0 B/op          0 allocs/op
Benchmark_Ctx_Format/json-8                              9499648               128.1 ns/op             0 B/op          0 allocs/op
Benchmark_Ctx_Format/json-8                              9510656               122.2 ns/op             0 B/op          0 allocs/op

Related to #2745

Migration

To make their code compatible with Fiber v3, users need only replace their uses of c.Format to c.AutoFormat.
To make full use of the new c.Format, users should note where in their application they are performing manual content negotiation and consider if c.Format will simplify their code.

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 (non-breaking change which adds functionality)
  • Documentation update (changes to documentation)

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.
  • Aimed for optimal performance with minimal allocations in the new code.
  • Provided benchmarks for the new code to analyze and improve upon.

While the existing Ctx.Format provides a concise convenience method for
basic content negotiation on simple structures, res.format allows
developers to set their own custom handlers for each content type.

The existing Ctx.Format is renamed to Ctx.AutoFormat.
@nickajacks1 nickajacks1 marked this pull request as ready for review December 17, 2023 21:43
@sixcolors
Copy link
Member

Thanks @nickajacks1 quick glance on mobile looks good. I will review tomorrow.

ctx.go Outdated Show resolved Hide resolved
ctx.go Show resolved Hide resolved
ctx.go Outdated Show resolved Hide resolved
ctx.go Outdated Show resolved Hide resolved
ctx.go Show resolved Hide resolved
ctx_interface.go Show resolved Hide resolved
ctx.go Outdated Show resolved Hide resolved
@ReneWerner87
Copy link
Member

thanks really cool improvement

- Rename Fmt to ResFmt
- Add comments in several places
- Return errors instead of panicking in Format
- Add 'Accept' to the Vary header in Format to match res.format
docs/api/ctx.md Outdated Show resolved Hide resolved
Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I missed it, but there's no test using a json object?

@nickajacks1
Copy link
Member Author

Not sure if I missed it, but there's no test using a json object?

Do you mean for Format or AutoFormat? For Format, there's not a lot of significance on specifically which content types are tested since you'll be calling c.JSON yourself.

For AutoFormat there's this:

	c.Request().Header.Set(HeaderAccept, MIMEApplicationJSON)
	err = c.AutoFormat("Hello, World!")
	require.NoError(t, err)
	require.Equal(t, `"Hello, World!"`, string(c.Response().Body()))

But maybe it would be nice to add something that takes advantage of struct tags to get better use-case coverage. I can add that tomorrow.

@ReneWerner87 ReneWerner87 merged commit 408fa20 into gofiber:v3-beta Jan 4, 2024
11 checks passed
@ReneWerner87 ReneWerner87 added this to the v3 milestone Jan 4, 2024
@ReneWerner87 ReneWerner87 linked an issue Jan 4, 2024 that may be closed by this pull request
3 tasks
@nickajacks1 nickajacks1 deleted the v3-resformat branch February 5, 2024 17:07
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.

📝 [v3 Proposal]: Parity for res.format and Ctx.Format
6 participants