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

Serve URL.prototype.toJSON to Safari <12 and Edge <18.17134 #1058

Merged
merged 5 commits into from
Jul 12, 2021

Conversation

romainmenke
Copy link
Collaborator

@romainmenke romainmenke commented May 29, 2021

URL polyfill is loaded in Safari <12 and Edge <18.17134.
This erases native support for URL.prototype.toJSON.

Matching config for should resolve this.

Screenshot 2021-05-29 at 10 34 28

https://mrhenry.github.io/web-tests/#59933cd3-48cc-4f36-9c52-9ac7b46e5329

@romainmenke romainmenke requested a review from a team as a code owner May 29, 2021 08:36
@origamiserviceuser origamiserviceuser added this to incoming in Origami ✨ May 29, 2021
@github-actions github-actions bot added the library Relates to an Origami library label May 29, 2021
@romainmenke romainmenke changed the title Serve URL.prototype.toJSON to Safari <12 Serve URL.prototype.toJSON to Safari <12 and Edge <18.17134 May 29, 2021
@romainmenke
Copy link
Collaborator Author

iOS also has unexpected results.
I haven't had the time yet to look into it.

Screenshot 2021-06-08 at 18 06 49

@romainmenke romainmenke marked this pull request as draft June 8, 2021 16:07
@JakeChampion JakeChampion removed the request for review from a team June 9, 2021 10:31
@JakeChampion JakeChampion removed this from incoming in Origami ✨ Jun 9, 2021
@romainmenke
Copy link
Collaborator Author

@JakeChampion Manually verified that iOS 12 and 13 would work after the config change.
These now also receive the URL polyfill, but not URL.prototype.toJSON

If I am not mistaken they receive the polyfill even when config says <12 because of the user agent parser.

@romainmenke romainmenke marked this pull request as ready for review June 13, 2021 11:14
@romainmenke
Copy link
Collaborator Author

Flaky test run in Edge I think.

@romainmenke
Copy link
Collaborator Author

@JakeChampion could you rerun CI on this one?
It should be ready to ship :)

Thank you

@JakeChampion
Copy link
Owner

@romainmenke running it now 👍

@JakeChampion
Copy link
Owner

@romainmenke I'm not sure it is a flaky test, I've rerun it several times now and it is always the same browser version and test which fails

@romainmenke
Copy link
Collaborator Author

I think I know why it's failing.
The tests run against Edge 18 based on test/polyfills/browserstackBrowsers.toml

[[browsers]]
os = "Windows"
os_version = "10"
browser = "edge"
browser_version = "18.0"

But the polyfill config has edge = "<18.17134".
The browser from browserstack has a version higher than 18.17134.

This is causing issues.

Thank you for running those tests. I assumed it was flaky because it was a seemingly unrelated polyfill.

I'll get back to you with a fix.

@origamiserviceuser origamiserviceuser added this to incoming in Origami ✨ Jul 12, 2021
@romainmenke
Copy link
Collaborator Author

romainmenke commented Jul 12, 2021

That should do it.

If the combination of the test UA and the feature query params results in an empty list we now fallback to the full list for the current UA.

It is never wrong to run the full list for a UA as running a subset is just an optimization to make CI faster.

This PR will run all tests anyway now because test/polyfills/server.js was changed.
You can test this locally with http://bs-local.com:9876/?includePolyfills=yes&always=no&feature=URL.prototype.toJSON against Edge 18 in browserstack.

@JakeChampion
Copy link
Owner

@romainmenke running on ci now 👍

@romainmenke
Copy link
Collaborator Author

All green now :)

@JakeChampion JakeChampion enabled auto-merge (rebase) July 12, 2021 18:42
auto-merge was automatically disabled July 12, 2021 18:42

Rebase failed

Origami ✨ automation moved this from incoming to active Jul 12, 2021
@JakeChampion JakeChampion merged commit b2c20e2 into JakeChampion:master Jul 12, 2021
Origami ✨ automation moved this from active to complete Jul 12, 2021
@romainmenke romainmenke deleted the patch-7 branch July 12, 2021 19:09
@romainmenke
Copy link
Collaborator Author

Thank you @JakeChampion! 🎉

I noticed that the last version wasn't released yet on npm.
Maybe these latest changes can be released as well?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 14, 2022
@robertboulton robertboulton removed this from Done in Origami ✨ Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
library Relates to an Origami library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants