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

Implement context.Context in App.Ctx. #383

Closed
wants to merge 2 commits into from
Closed

Implement context.Context in App.Ctx. #383

wants to merge 2 commits into from

Conversation

hsblhsn
Copy link

@hsblhsn hsblhsn commented May 16, 2020

@welcome
Copy link

welcome bot commented May 16, 2020

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@hsblhsn
Copy link
Author

hsblhsn commented May 16, 2020

Someone please fix the failed github workflow file.

@Fenny
Copy link
Member

Fenny commented May 16, 2020

@boolooper thank you for your first PR, Fiber supports getting/setting context values using ctx.Locals().

Please take a minute to read our documentation https://docs.gofiber.io or visit our examples https://github.com/gofiber/recipes. In these docs you can also find the ctx.Get() method you asked for in #382 (comment)

@hsblhsn
Copy link
Author

hsblhsn commented May 16, 2020

This PR does not only adds "passing context values". It's about implementing the context.Context interface. Many libraries takes context.Context as parameter including standard database/sql.

The goal of this PR is supporting App.Ctx for passing as a parameter to functions line sql.Conn.BeginTx.

context.Context is being used in lots of places in go ecosystem. Even fasthttp.RequestCtx implements this.

@Fenny
Copy link
Member

Fenny commented May 16, 2020

Thanks for your proposal and use-case example.

Fiber almost implements an one on one expressjs like API. We are open for improvements/features to be added to the Ctx. I'm not sure if it's worth to add the context.Context interface directly to the Fiber Ctx.

In the end Fiber is a simplified layer build around Fasthttp, so if you really need to pass a context interface you could use ctx.Fasthttp (fasthttp.RequestCtx).

app.Get("/", func(c *fiber.Ctx) {
	db.QueryContext(c.Fasthttp, "SELECT name FROM users WHERE age=?", 25)
})

My main concern is that if we add ctx.Done(), ctx.Value() it might complicate the current API design and I'm not sure if that's worth it.

Feel free to share your thoughts, I will leave this PR open for discussion and invite the @gofiber/maintainers to join!

Feel free to chat with us on discord https://gofiber.io/discord

@hsblhsn
Copy link
Author

hsblhsn commented May 17, 2020

Just saw, fiber.Ctx implements the io.Writer interface by adding Ctx.Write method ignoring express API design. I don't think adding context.Context will be a problem.

@thomasvvugt thomasvvugt added this to In progress in Development May 17, 2020
@Fenny
Copy link
Member

Fenny commented May 19, 2020

write is a method provided by the OutgoingMessage class from the node http module. Express.js inherits this class and therefore we implement the namespace in our API.

// express
app.get('/user/:id', function (req, res) {
  res.write('<html>');
  res.write('<body>');
  res.write('<h1>Hello, World!</h1>');
  res.write('</body>');
  res.write('</html>');
  res.end()
});
// fiber
app.Get("/user/:id", func(ctx *fiber.Ctx) {
  ctx.Write("<html>");
  ctx.Write("<body>");
  ctx.Write("<h1>Hello, World!</h1>");
  ctx.Write("</body>");
  ctx.Write("</html>");
});

I'm closing this PR for now since an alternative solution is provided #383 (comment)
Until we have a real use-case to build the Context on top of the Fiber Ctx this will not be implemented. Thank you @boolooper for your idea and PR!

@Fenny Fenny closed this May 20, 2020
Development automation moved this from In progress to Done May 20, 2020
@hsblhsn
Copy link
Author

hsblhsn commented May 21, 2020

But I would really request you to provide a function like Ctx.Context that returns an implementation of context.Context.
Passing ctx.Fasthttp as a context.Context really looks weird in code.
A Context() method in fiber.Ctx would be really great and self documented than ctx.Fasthttp.

Moving from fasthttp to fiber is making my code look weird for this silly reason.
Please for the sake of readable code, provide a function like this or implement context.Context in fiber.Ctx.

Best regards.

@Fenny Fenny reopened this May 22, 2020
Development automation moved this from Done to In progress May 22, 2020
@Fenny
Copy link
Member

Fenny commented May 22, 2020

You got a valid point, if the @gofiber/maintainers agree we could merge it if a test case is also provided.

@thomasvvugt
Copy link
Contributor

@gofiber/maintainers LGTM, I would like to see this merged in 1.10 as this would break all working code having the ctx.Context naming.

@Fenny Fenny mentioned this pull request May 23, 2020
@Fenny
Copy link
Member

Fenny commented May 23, 2020

Added in v1.10.0 under ctx.Context()

@Fenny Fenny closed this May 23, 2020
Development automation moved this from In progress to Done May 23, 2020
@Fenny Fenny mentioned this pull request May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants