-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update ReadableStream compatibility in Safari #5424
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also update the constructor on lines 148 and 151
Hey there, thanks for your PR! Unfortunately, the docs that you are basing the version numbers on don't actually refer to the Safari versions, and are inaccurate. As such, we will not be able to merge this PR as it is. There's two ways we can proceed. One is to try to find all the actual versions through SauceLabs / BrowserStack / etc., and the other one is to set everything to |
@vinyldarkscratch Thank you for the advice. I just tested the ReadableStream apis(https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=6670) on real devices on SourceLabs and mobile. Here's the screenshot: |
@vinyldarkscratch Can you take a look at my changes? This is my first time contributing to this Repo. Let me know if I'm still missing anything |
Don’t worry, I’ve requested a review for myself so I’ll check out your changes when I can! (I’m the only one in the office for my day job so I have to work double time.) |
@vinyldarkscratch Any more update is needed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey again, sorry for the delays in my review! I've been caught up with a lot of projects regarding this repository, as well as being sick with the flu and preparing for Mozilla All Hands 2020, so I hadn't had much time to review PRs. I just got back to this, and was able to double-check the versions.
It appears that this API was specifically implemented in Safari 10.1 (which equates to Safari for iOS 10.3). Safari 10.0 doesn't have support. (Unfortunately SauceLabs doesn't distinguish the two versions well; the only way to change which one to use is the OS version.) Can we bump the version number to "10.1" for Safari Desktop, and "10.3" for Safari iOS?
Hey @AllanFly120, do you plan to return to this PR? |
Marking this as inactive since we haven't heard back from you. If we don't hear back in another week, I'll go ahead and supersede this PR so we can get the changes going! |
I went ahead and opened #5738 to continue the updates to the Safari data. Thank you for your PR! |
A checklist to help your pull request get merged faster:
According to Apple developer guide, ReadableStream is supported. Currently, this API is shown as not supported in MDN. The source of these changes come from Apple Developer Guide as below:
Tested on safari 13 and safari_ios 13 with following cases:
https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=6670