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

test in Firefox on CI #169

Merged
merged 2 commits into from
Mar 29, 2019
Merged

test in Firefox on CI #169

merged 2 commits into from
Mar 29, 2019

Conversation

marcoow
Copy link
Member

@marcoow marcoow commented Apr 4, 2018

This adds Firefox to the browsers we test with in CI; see #152 (comment)

It also includes a fix for a bug (or different behavior compared to Chrome) in Firefox: Firefox will set the default cookie path with a trailing slash (e.g. /some/path/) while Chrome would set it without the trailing slash (e.g. /some/path). In order to make the behavior the same for all browsers, this changes the cookies service so that it would automatically set a normalized default path (/some/path in the above example) if none was specified. I'm not really happy with this (as writing a cookie via the cookies service will now result in a different cookie than just using document.cookie =) but couldn't really think of a better solution 🤔

@marcoow marcoow requested a review from geekygrappler April 4, 2018 16:43
@marcoow marcoow requested review from a team and removed request for geekygrappler March 8, 2019 09:29
@Turbo87
Copy link
Collaborator

Turbo87 commented Mar 8, 2019

IMHO the bugfix belongs in a separate PR, but since we're writing a manual changelog here it might not matter that much. still makes it somewhat harder to review though...

@marcoow
Copy link
Member Author

marcoow commented Mar 8, 2019

@simplabs/engineering anyone care to review?

Copy link
Contributor

@jayjayjpg jayjayjpg left a comment

Choose a reason for hiding this comment

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

@marcoow This seems great 👍 Do you happen to have a reference for the Firefox bug about the trailing slash that you mentioned as well? I'm not sure I fully understand the bug yet

@marcoow
Copy link
Member Author

marcoow commented Mar 28, 2019

@jessica-jordan I don't have a reference and I'm not sure it's really a bug in Firefox. It's something where it behaves differently than other browsers but I'm not sure what the exact spec is for this or whether there is a spec that covers this at all. The tests fail in Firefox though without this change.

@jayjayjpg
Copy link
Contributor

Thank you for the outline, I think you're right that it makes most sense to provide a fallback path then 👍

@marcoow marcoow merged commit 2dd087c into master Mar 29, 2019
@delete-merged-branch delete-merged-branch bot deleted the test-firefox branch March 29, 2019 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants