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

Update ReadableStream compatibility in Safari #5424

Closed
wants to merge 5 commits into from

Conversation

AllanZhengYP
Copy link
Contributor

@AllanZhengYP AllanZhengYP commented Dec 27, 2019

A checklist to help your pull request get merged faster:

  • Summarize your changes
  • Data: link to resources that verify support information (such as browser's docs, changelogs, source control, bug trackers, and tests)
  • Data: if you tested something, describe how you tested with details like browser and version
  • Review the results of the linter and fix problems reported (If you need help, please ask in a comment!)
  • Link to related issues or pull requests, if any

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

@ghost ghost added the data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Dec 27, 2019
Copy link

@trivikr trivikr left a 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

api/ReadableStream.json Outdated Show resolved Hide resolved
@queengooborg
Copy link
Contributor

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 true for now.

@queengooborg queengooborg added the not ready This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. label Dec 28, 2019
@AllanZhengYP
Copy link
Contributor Author

@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:

  • desktop safari:
    Screen Shot 2020-01-06 at 10 24 12 AM
  • mobile safari:
    Live DOM Viewer

@queengooborg queengooborg self-requested a review January 6, 2020 20:26
@AllanZhengYP
Copy link
Contributor Author

@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

@queengooborg
Copy link
Contributor

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.)

@AllanZhengYP
Copy link
Contributor Author

@vinyldarkscratch Any more update is needed?

@queengooborg queengooborg removed the not ready This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. label Jan 15, 2020
Copy link
Contributor

@queengooborg queengooborg left a 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?

@queengooborg
Copy link
Contributor

Hey @AllanFly120, do you plan to return to this PR?

@queengooborg
Copy link
Contributor

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!

@queengooborg
Copy link
Contributor

I went ahead and opened #5738 to continue the updates to the Safari data. Thank you for your PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants