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 domain when deleting cookies #5341

Merged
merged 8 commits into from
Dec 14, 2022

Conversation

alexpdraper
Copy link
Contributor

Changes

Adds domain as a property on the delete cookie options. Without this it wasn’t possible to use Astro.cookies.delete to delete a cookie that has the domain set.

Testing

Added a unit test that checks the domain is added to the set-cookie header when provided, similar to the test for the path option.

Docs

/cc @withastro/maintainers-docs for feedback!
The API reference for Astro.cookies.delete doesn’t include the options as one of the arguments.

https://github.com/withastro/docs/blob/main/src/pages/en/reference/api-reference.md#astrocookies

@changeset-bot
Copy link

changeset-bot bot commented Nov 9, 2022

🦋 Changeset detected

Latest commit: 01e6d1d

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 the pkg: astro Related to the core `astro` package (scope) label Nov 9, 2022
@matthewp
Copy link
Contributor

The addition makes sense to me. We will indeed need docs before merging this. The docs for that are here: https://github.com/withastro/docs/blob/da3725bf4e3a24647ee240a3816cc5d72e31d9e6/src/pages/en/reference/api-reference.md#astrocookies

@matthewp
Copy link
Contributor

Pretty sure the tests failing is unrelated to your change and it's a problem in main. I'm looking to fix that, so please disregard.

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.

  • Should be a minor change.
  • Needs docs

@matthewp
Copy link
Contributor

Thanks @alexpdraper, when you have a docs PR please link to that from here. Then the docs team will review it and approve this PR.

@matthewp matthewp requested a review from a team November 10, 2022 22:15
@sarah11918
Copy link
Member

This looks like a useful feature @alexpdraper! You're on our radar now at docs, and we'll be on the lookout for a PR.

Please PR a suggested update to this section of the docs: https://docs.astro.build/en/reference/api-reference/#astrocookies (file is at: https://github.com/withastro/docs/blob/main/src/pages/en/reference/api-reference.md )

and we will help over there with final wording, formatting, and just generally squishing it in to that table!

@matthewp
Copy link
Contributor

matthewp commented Dec 8, 2022

@alexpdraper Are you planning on making the docs changes?

@matthewp matthewp closed this Dec 8, 2022
@matthewp
Copy link
Contributor

matthewp commented Dec 8, 2022

Oops, closed by mistake.

@matthewp matthewp reopened this Dec 8, 2022
alexpdraper added a commit to alexpdraper/docs that referenced this pull request Dec 8, 2022
@alexpdraper
Copy link
Contributor Author

@alexpdraper Are you planning on making the docs changes?

Yes, certainly. Sorry about the delay.

@sarah11918 here’s the docs PR withastro/docs#2151

@sarah11918
Copy link
Member

Docs PR received! Removing Astro-Docs maintainers requirement for review here, and Docs will take this up over there. 🚀

@sarah11918 sarah11918 removed the request for review from a team December 9, 2022 15:20
@alexpdraper alexpdraper requested a review from matthewp December 12, 2022 21:11
@matthewp matthewp added the semver: minor Change triggers a `minor` release label Dec 12, 2022
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.

Looks good, this will go out with the next minor (later this week)

@alexpdraper
Copy link
Contributor Author

Great! Thanks so much!

@matthewp matthewp dismissed their stale review December 14, 2022 15:24

Today is merge day

@matthewp matthewp merged commit 6b156dd into withastro:main Dec 14, 2022
@astrobot-houston astrobot-houston mentioned this pull request Dec 14, 2022
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) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants