Global DefaultOptions

[syntaqx]

The intent of this is to enable you to globally set DefaultOptions so that it's not strictly necessary to set them across usages of cookie.Set

Additionally, this should allow for use of signed to be true by default.

TODO

• This requires PopulateFromCookies to be able to understand unsigned as well when the default value is signed.
• Validate that the default options are the same as the net/http package

Open to feedback during early stages of this PR.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@03ac385). Learn more about missing BASE report.
Report is 3 commits behind head on main.

❗ Current head f222e6a differs from pull request most recent head f120b07

Please upload reports for the commit f120b07 to get more accurate results.

Additional details and impacted files

@@           Coverage Diff            @@
##             main        #4   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?         1           
  Lines           ?       146           
  Branches        ?         0           
========================================
  Hits            ?       146           
  Misses          ?         0           
  Partials        ?         0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[syntaqx]

Leaving the pull request open for feedback for a while before merging!

[ccoVeille]

I'm a bit afraid by such behavior. It could lead to race conditions if your lib is used by multiple libs.

I would recommend a pattern that will be immutable

Something like

mySignedCookieHelper := cookie.WithOptions( /* */)

mySignedCookieHelper.SetCookie("foo", "bar")

Also without such a thing, tests could become a nightmare as you are accessing sane global variable.

Otherwise, you will need to use mutex lock/unlock.

[ccoVeille]

@syntaqx

What do you think about this?

[syntaqx]

How would this lead to race conditions?

[ccoVeille]

cookie.WithOptions would return a soped variable and no global variable will be used.

[syntaqx]

Sure, but that's an entire refactor of the package to introduce instanced cookie managers rather than a global one...

[syntaqx]

I'm not necessarily entirely against this, but it's a pretty massive change in direction.

[ccoVeille]

I agree. It's put of the scope of your change. But, unless if I'm wrong. Before this PR there was no global variable that could cause race condition.

I think it's a needed refactoring, but yes, it requires to change a lot of things.

[syntaqx]

I encourage you to check out #8 as I'm currently exploring what this might look like. I think the ideal of having a DefaultManager available so package-level functions continue to work is ideal, but all of this is why I'm still v0 of my package 😆

[ccoVeille]

I do appreciate.

For records, I'm not even sure current implementation, so not what you are doing with #8, would have passed tests if you used go test -race ./...