Skip to content

cookie.Manager and DefaultManager #8

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

Merged
merged 30 commits into from
Jul 2, 2024
Merged

cookie.Manager and DefaultManager #8

merged 30 commits into from
Jul 2, 2024

Conversation

syntaqx
Copy link
Owner

@syntaqx syntaqx commented Jul 2, 2024

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.

@syntaqx syntaqx mentioned this pull request Jul 2, 2024
2 tasks
Copy link

codecov bot commented Jul 2, 2024

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
Copy link

I appreciate you consider this rewriting

@syntaqx
Copy link
Owner Author

syntaqx commented Jul 2, 2024

@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 syntaqx self-assigned this Jul 2, 2024
@syntaqx syntaqx added the enhancement New feature or request label Jul 2, 2024
@syntaqx syntaqx marked this pull request as ready for review July 2, 2024 08:58
@syntaqx
Copy link
Owner Author

syntaqx commented Jul 2, 2024

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.

@syntaqx syntaqx changed the title Initial pass using a Cookie Manager rather than global objects cookie.Manager and DefaultManager Jul 2, 2024
Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

Small remark about README, except that 👍