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.

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

Allow setting all cookie package serialize/parse options #9063

Merged
merged 12 commits into from
Jan 3, 2024
Merged

Allow setting all cookie package serialize/parse options #9063

merged 12 commits into from
Jan 3, 2024

Conversation

alex-sherwin
Copy link
Contributor

@alex-sherwin alex-sherwin commented Nov 11, 2023

This PR fixes #9062

  • Changes AstroCookieSetOptions to be based on CookieSerializeOptions from the cookie package. All the Astro code was already passing the options provided directly to cookie's serialize function, so this is just a typing change.
  • Changes AstroCookieSetOptions to be exported from the main astro package so it can be utilized by end-user code
  • Adds an exported AstroCookieGetOptions based on CookieParseOptions from the cookie package.
  • Modifies the Astro.cookies.get() function to take an optional options: AstroCookieGetOptions | undefined which is passed down to the cookie package's parse function
  • Astro.cookies.has() gets the same treatment as get since it triggers cookie parsing
  • Test coverage for existing behavior which covers the fact that the default behavior of cookie set/get value serialization is to URI encode the value (the cookie package is doing this by default, but no tests were explicitly covering this behavior)
  • Test coverage for overriding the cookie set/get behavior for serialization/deserialization with identity functions to prevent URI encoding/decoding

Copy link

changeset-bot bot commented Nov 11, 2023

🦋 Changeset detected

Latest 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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Nov 11, 2023
@alex-sherwin alex-sherwin changed the title Fix 9062: allow setting all cookie package serialize/parse options Allow setting all cookie package serialize/parse options Nov 11, 2023
@matthewp
Copy link
Contributor

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!

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Instead of inheriting from CookieSerializeOptions, just take the properties it has that we need.

@alex-sherwin
Copy link
Contributor Author

alex-sherwin commented Nov 17, 2023

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!

Happy to make that change @matthewp

But to clarify, you're receptive to exposing at least the encode and decode options (and thus commit to supporting these in Astro in the event the underlying serialization package/dependency changes in the future)?

I don't think that should be a problem, the barrier is low (the behavior now is to delegate to the encodeURIComponent and decodeURIComponent builtins

@matthewp
Copy link
Contributor

@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.

@matthewp matthewp added the semver: minor Change triggers a `minor` release label Nov 17, 2023
@matthewp
Copy link
Contributor

Hello @alex-sherwin, was wondering if you planned to update this PR?

@alex-sherwin
Copy link
Contributor Author

Hello @alex-sherwin, was wondering if you planned to update this PR?

Hi @matthewp, yes, I will plan to update it today

@florian-lefebvre
Copy link
Member

@alex-sherwin up! ;)

@matthewp
Copy link
Contributor

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.

@Princesseuh Princesseuh added this to the 4.1.0 milestone Jan 2, 2024
@matthewp
Copy link
Contributor

matthewp commented Jan 2, 2024

Docs PR: withastro/docs#6042

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thanks so much for providing this, @matthewp ! Looks great! Just left some tiny suggestions for your consideration!

.changeset/tiny-days-dance.md Outdated Show resolved Hide resolved
.changeset/tiny-days-dance.md Outdated Show resolved Hide resolved
Co-authored-by: Sarah Rainsberger <[email protected]>
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Added a better suggestion for the changelog. The arrow function o => o didn't tell us very much

.changeset/tiny-days-dance.md Outdated Show resolved Hide resolved
.changeset/tiny-days-dance.md Outdated Show resolved Hide resolved
matthewp and others added 2 commits January 3, 2024 08:00
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Approving on behalf of docs for the sake of completion/visibility!

@ematipico ematipico merged commit f33fe31 into withastro:main Jan 3, 2024
13 of 14 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Astro.cookies.set() does not expose underlying cookie serialize options
8 participants