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

feat: make cookies have sameSite key by default #7790

Merged
merged 4 commits into from Jun 29, 2020
Merged

Conversation

flotwig
Copy link
Contributor

@flotwig flotwig commented Jun 23, 2020

BREAKING CHANGE: modifies the shape of Cookie objects

User facing changelog

  • Values yielded by cy.setCookie, cy.getCookie, and cy.getCookies will now always contain a sameSite attribute.
  • Removed experimentalGetCookiesSameSite configuration flag, since this behavior is now the default.

Additional details

How has the user experience changed?

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 23, 2020

Thanks for taking the time to open a PR!

BREAKING CHANGE: modifies the shape of Cookie objects
@cypress
Copy link

cypress bot commented Jun 23, 2020



Test summary

7816 0 129 0


Run details

Project cypress
Status Passed
Commit 03c5ce4
Started Jun 29, 2020 6:29 PM
Ended Jun 29, 2020 6:37 PM
Duration 08:02 💡
OS Linux Debian - 10.1
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@flotwig flotwig marked this pull request as ready for review June 25, 2020 20:38
@flotwig flotwig requested review from a team, brian-mann and kuceb and removed request for a team June 25, 2020 20:39
@jennifer-shehane jennifer-shehane added the type: breaking change Requires a new major release version label Jun 26, 2020
@flotwig flotwig mentioned this pull request Jun 26, 2020
21 tasks
Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

What happens if a user still uses experimentalGetCookiesSameSite in the config even though it's been removed?

Should they get a warning or should it error or silently do nothing?

@flotwig
Copy link
Contributor Author

flotwig commented Jun 29, 2020

I think that since users with experimentalGetCookiesSameSite enabled will not experience a change in behavior, we don't need to add a deprecation warning at runtime. The migration guide will presumably document that this config option can be removed, and we should probably comment on the closed issues that this is no longer "experimental", and the flag is no longer necessary.

@brian-mann
Copy link
Member

I think in this case that's probably fine, but since we're doing the experimental* pattern more frequently now it makes me wonder what our best practice should be. I'm actually on the side that we should hard error and throw and tell them this option has been removed.

I think that approach is much better than to silently pass because anyone looking at the code later will be sufficiently confused - won't be able to find the option anywhere in the docs, and will wonder if this is actually taking effect anywhere. Throwing forces the user to update their code, but it's the cleanest way to communicate the intention. Additionally, imagine if we had a situation where the options of the experimental* feature change to have new official meaning when it gets released.

I can think of a few other scenarios but those are the ones off the top of my head. I believe we already have logic in config to notify the user of renamed / removed properties. I'd also be okay with a deprecation notice, but unless people literally read their stdout they won't see it.

@flotwig
Copy link
Contributor Author

flotwig commented Jun 29, 2020

I guess it's either make the user pay the cost now, by forcing them to update their config, or pay the cost later, by potentially confusing users that have not read the migration guide/seen the issue comment telling them to remove the config option.

Adding a deprecation notice to stdout sounds like a decent compromise between the two that will help make the 5.x upgrade easier, so that's what I'll do for this PR.

@brian-mann
Copy link
Member

Okay cool. Use the existing config warning/deprecation machinery.

Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

See the discussion here: #7790 (comment)

@flotwig flotwig requested a review from brian-mann June 29, 2020 16:39
@@ -923,6 +923,11 @@ const getMsgByType = function (type, arg1 = {}, arg2, arg3) {
Enable write permissions to this directory to ensure screenshots and videos are stored.

If you don't require screenshots or videos to be stored you can safely ignore this warning.`
case 'EXPERIMENTAL_SAMESITE_REMOVED':
return stripIndent`\
The \`experimentalGetCookiesSameSite\` configuration option was removed in Cypress 5. Yielding the \`sameSite\` property is now the default behavior of the \`cy.cookie\` commands.
Copy link
Member

Choose a reason for hiding this comment

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

I think the convention when listing versions is to say in Cypress version x.x.x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the only prior art I could find, which also isn't that:

Note: In Cypress 4.0, Canary must be launched as \`chrome:canary\`, not \`canary\`.

Copy link
Member

Choose a reason for hiding this comment

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

In our migration guide we usually say Cypress 5, so as not to be restricted of it only applying to Cypress 5.0.0 and not to 5.1.0, etc.

@flotwig flotwig deleted the issue-6892 branch January 24, 2022 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: breaking change Requires a new major release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make adding sameSite value to objects yielded by cy.getCookie/setCookie/getCookies the default
3 participants