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.

Sign up for GitHub

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

Jump to bottom

🔥 Adding user context #1341

Merged
merged 5 commits into from
May 24, 2021
Merged

🔥 Adding user context #1341

merged 5 commits into from
May 24, 2021

Conversation

patil-kshitij
Copy link
Contributor

Resolved #1249

This adds an additional field called userContext which is of type context.Context.
It could be set by user.

@welcome
Copy link

welcome bot commented May 19, 2021

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

@patil-kshitij patil-kshitij changed the title Adding user context 🔥 Adding user context #1249 May 19, 2021
@patil-kshitij patil-kshitij changed the title 🔥 Adding user context #1249 🔥 Adding user context May 19, 2021
@liaohongxing
Copy link
Contributor

liaohongxing commented May 20, 2021 •
edited
Loading

  1. expressjs styles
c.UserContext(context.Background)   // set Context
c.UserContext()                     // get Context
  1. why
c.Locals("c", context.Background)   // setContext
c.Locals("c")                       // getContext

@patil-kshitij
Copy link
Contributor Author

  1. Should I write setting and getting context in expressjs styles ?

  2. We need context propagation, where Go defined context interface's implementation could be set by user.

I could implement point 1.
Please let me know if you are okay with point2.

@liaohongxing
Copy link
Contributor

This is just my suggestion, listen to the suggestions of other mainline maintainers

@ReneWerner87
Copy link
Member

ReneWerner87 commented May 20, 2021 •
edited
Loading

@patil-kshitij please add something in the README for this or/and the docs repository

only every known feature is used 😄

ReneWerner87
ReneWerner87 reviewed May 20, 2021
View reviewed changes
ctx.go
@@ -309,6 +311,17 @@ func (c *Ctx) Context() *fasthttp.RequestCtx {
return c.fasthttp
}

func (c *Ctx) UserContext() context.Context {
Copy link
Member

Choose a reason for hiding this comment

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

add a method docblock with description

we receive a lot of support requests and i think this could eliminate some requests from the start

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done,
Kindly review

ReneWerner87
ReneWerner87 reviewed May 20, 2021
View reviewed changes
ctx.go
return c.userContext
}

func (c *Ctx) SetUserContext(ctx context.Context) {
Copy link
Member

Choose a reason for hiding this comment

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

add a method docblock with description

we receive a lot of support requests and i think this could eliminate some requests from the start

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done,
Kindly review

Copy link
Contributor

@hi019 hi019 May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Need a space after the last /. Same with the test functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done
Kindly review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @hi019,
Added space after last /.
Please review

@patil-kshitij
Copy link
Contributor Author

patil-kshitij commented May 21, 2021 •
edited
Loading

@ReneWerner87, please let me know if, I should add a parallel PR on docs to add documentation regarding setting and getting user-context, which could be used for observability purposes?

@ReneWerner87
Copy link
Member

Yes please add a PR in the docs repository.

@patil-kshitij
Copy link
Contributor Author

Hi @ReneWerner87 ,
Have raised PR for documentation as well.
Kindly review, gofiber/docs#173

@ReneWerner87
Copy link
Member

Will check it in a few hours

@patil-kshitij
Copy link
Contributor Author

patil-kshitij commented May 24, 2021 •
edited
Loading

Hi @ReneWerner87, @hi019
Can we please merge this PR if review is completed ?

@hi019 hi019 self-requested a review May 24, 2021 04:38
@hi019 hi019 merged commit 8616b41 into gofiber:master May 24, 2021
@welcome
Copy link

welcome bot commented May 24, 2021

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87
Copy link
Member

@patil-kshitij could you check https://github.com/gofiber/fiber/pull/1522/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ReneWerner87 ReneWerner87 ReneWerner87 approved these changes

@hi019 hi019 hi019 approved these changes

Assignees
No one assigned
Labels
✏️ Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🤗 Context Propagation
4 participants
@patil-kshitij @liaohongxing @ReneWerner87 @hi019