Allow setting all cookie package serialize/parse options#9063
Allow setting all cookie package serialize/parse options#9063ematipico merged 12 commits intowithastro:mainfrom alex-sherwin:main
Conversation
🦋 Changeset detectedLatest commit: d41b619 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for the contribution @alex-sherwin! We don't normally like to expose dependency options like this, because it leaks an implementation detail. We would like to be able to swap out the serialization dependency without it being a breaking change in Astro. So, instead what I would suggest is adding the properties from CookieSerializeOptions that you need directly to AstroCookieSetOptions, so that it's part of our explicit API. Thanks! |
matthewp
left a comment
There was a problem hiding this comment.
Instead of inheriting from CookieSerializeOptions, just take the properties it has that we need.
Happy to make that change @matthewp But to clarify, you're receptive to exposing at least the I don't think that should be a problem, the barrier is low (the behavior now is to delegate to the |
|
@alex-sherwin Yeah, totally fine with exposing such an option, just didn't want to expose all of the dependencies' options for the backwards compat reason. |
|
Hello @alex-sherwin, was wondering if you planned to update this PR? |
Hi @matthewp, yes, I will plan to update it today |
|
@alex-sherwin up! ;) |
|
Great job getting this over the line @florian-lefebvre. We don't currently have a plan for the next minor, it will likely come around the new year. |
|
Docs PR: withastro/docs#6042 |
sarah11918
left a comment
There was a problem hiding this comment.
Thanks so much for providing this, @matthewp ! Looks great! Just left some tiny suggestions for your consideration!
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
ematipico
left a comment
There was a problem hiding this comment.
Added a better suggestion for the changelog. The arrow function o => o didn't tell us very much
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
sarah11918
left a comment
There was a problem hiding this comment.
Approving on behalf of docs for the sake of completion/visibility!
This PR fixes #9062
AstroCookieSetOptionsto be based onCookieSerializeOptionsfrom the cookie package. All the Astro code was already passing the options provided directly to cookie'sserializefunction, so this is just a typing change.AstroCookieSetOptionsto be exported from the main astro package so it can be utilized by end-user codeAstroCookieGetOptionsbased onCookieParseOptionsfrom the cookie package.Astro.cookies.get()function to take an optionaloptions: AstroCookieGetOptions | undefinedwhich is passed down to the cookie package's parse functionAstro.cookies.has()gets the same treatment asgetsince it triggers cookie parsing