cookie.Manager and DefaultManager

[syntaqx]

Based on some changes in the latest version, as well as some optimizations I think we're able to do to the package overall, this changeset contains the following major changes:

• cookie.Manager is now the primary API to the package
• cookie.DefaultManager is initialized and provides API stability for the global Set/Get/... methods.
• Options API Change:
  • cookie.NewManager(cookie.WithSigningKey(signingKey))
• Introduction of CustomTypeHandler so we no longer impose a specific dependency (gofrs/uuid)
• Significant upgrade to the PopulateFromCookies functionality
• File organization for better separation of functionality and tests
• Removal of DefaultOptions - The mergeOptions functionality wasn't working well anyways.
• Adds omitempty to silently ignore missing cookies.

While I hope this introduces the least amount of API breakage as possible, I hope the new API will be significantly more stable than the previous iterations and this can be the foundation for moving forward.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b7e9622) to head (06ba184).

Additional details and impacted files

@@            Coverage Diff            @@
##              main        #8   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         5    +4     
  Lines          146       189   +43     
=========================================
+ Hits           146       189   +43     

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

[ccoVeille]

I appreciate you consider this rewriting

[syntaqx]

@ccoVeille We'll see how the experiment goes, but I'm certainly willing to entertain it for now.

I've finished my initial pass of just the Cookie Manager. I'll start looking at what it might take to introduce a DefaultManager that allows for API compatibility with the global method usage, but allows the use to run off the default, or specific managers.

Would love any feedback from the first pass.

[syntaqx]

It seems to me that providing the DefaultManager will work for the things I wanted it to do, and fixes the issues you were seeing. Tests are -race in CI, and I'm not seeing any major issues.

I'll leave this open for feedback while I sleep and check back in, and maybe we can get this merged in if I don't come up with any reasons not to between now and then.

[ccoVeille]

Small remark about README, except that 👍

[ccoVeille]

I don't think this one has to be exported

[syntaqx]

This way it's overrideable, so if you want to set custom functions or a custom signing key you can set the value

[syntaqx]

Usage of this is documented in the README

[ccoVeille]

OK, please let me check one more thing

[ccoVeille]

OK, I was a bit afraid the DefaultManager would be altered by one of the Manager methods.

It seems OK.
There is no such a thing as:

• get the default
• set something that could alter the default
• any code relying on same lib, would have an altered default

The only way to alter it is via the NewManager. So it's fine.

Maybe a test could cover that no matter how many calls you do to public method, defaultmanager is untouched.

[syntaqx]

I'm not totally sure what you mean, it's perfectly okay for the user of the library to touch the DefaultManager if they wish to override it with specific settings.

I'll update the example to showcase the intent of this.

[syntaqx]

@ccoVeille I'm pretty solid with getting this merged in pending the DefaultManager concerns you've mentioned, which I'm not totally sure I understand. If you can elaborate any remaining concerns around that if you still have them; Otherwise, I think we can get this merged. I'll probably release this as a v0.1.0 so it's clear this is fairly large.

[ccoVeille]

@ccoVeille I'm pretty solid with getting this merged in pending the DefaultManager concerns you've mentioned, which I'm not totally sure I understand. If you can elaborate any remaining concerns around that if you still have them; Otherwise, I think we can get this merged. I'll probably release this as a v0.1.0 so it's clear this is fairly large.

I'm sorry if it wasn't clear.

I was worried the DefaultManager could be affected in some way.

Something like cookie.DefaultManager.Set(whatever) that would affect the cookie.DefaultManager

I checked and I see nothing that could lead to that with current code.

The only thing that can alter a manager is when you create it with NewManager and pass Option. But once created, you cannot alter it. So it's immutable, so it's perfect.

I hope it's clearer, if not never mind, you can merge anyway.