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

Add Safari versions for IDB* API #10920

Merged
merged 13 commits into from
Jun 22, 2021
Merged

Add Safari versions for IDB* API #10920

merged 13 commits into from
Jun 22, 2021

Conversation

queengooborg
Copy link
Contributor

This PR adds real values for Safari (Desktop and iOS/iPadOS) for the IDB* API, based upon results from the mdn-bcd-collector project (v3.1.4).

Tests Used: https://mdn-bcd-collector.appspot.com/tests/api/IDB*

This PR adds real values for Safari (Desktop and iOS/iPadOS) for the `IDB*` API, based upon results from the [mdn-bcd-collector](https://mdn-bcd-collector.appspot.com) project (v3.1.4).

Tests Used: https://mdn-bcd-collector.appspot.com/tests/api/IDB*
@github-actions github-actions bot added the data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Jun 10, 2021
api/IDBCursorWithValue.json Outdated Show resolved Hide resolved
@queengooborg queengooborg self-assigned this Jun 14, 2021
@foolip
Copy link
Contributor

foolip commented Jun 17, 2021

@vinyldarkscratch there are conflicts now, I'll hold off with review until pinged.

@queengooborg
Copy link
Contributor Author

@foolip Ping!

Copy link
Contributor

@foolip foolip left a comment

Choose a reason for hiding this comment

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

With all the batch changes I asked more annoying questions about anything that didn't fit the pattern :)

api/IDBCursor.json Outdated Show resolved Hide resolved
api/IDBEnvironment.json Show resolved Hide resolved
@@ -966,10 +966,10 @@
"version_added": "14"
},
"safari": {
"version_added": "7"
"version_added": "10.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is openKeyCursor and is later than the rest. Is this based on mdn-bcd-collector results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is pretty much all from the collector results, aside from ensuring iOS stays marked as "8" and not earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't match https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/openKeyCursor, can you check if the data for that is also wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I've compared https://mdn-bcd-collector.appspot.com/tests/api/IDBObjectStore and https://mdn-bcd-collector.appspot.com/tests/api/IDBIndex in Safari 10 and 10.1, and there's a lot more than this that came in 10.1.

How about backing this out and making all of those changes as a single PR? I'm not sure I understand why the collector isn't updating all of the data, but it looks like api.IDBIndex.openKeyCursor was in Safari 8 and just temporarily gone. Something weird happened at any rate, which is better to split out.

@@ -661,10 +661,10 @@
"version_added": "35"
},
"safari": {
"version_added": true
"version_added": "10.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is objectStoreNames, confirmed manually?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's back this out too, I'm no longer trusting of the 10.1 versions.

api/IDBVersionChangeEvent.json Outdated Show resolved Hide resolved
Copy link
Contributor

@foolip foolip left a comment

Choose a reason for hiding this comment

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

I've confirmed with some npm run traverse safari_ios api $version calls that there's no iOS data <8 now.

@foolip
Copy link
Contributor

foolip commented Jun 17, 2021

Hmm, now I'm having second thoughts in light of #9423, where we're contemplating just removing 6.1 and 6.2 as releases. If we do, we'd change these 6.1 back to 7 again.

So... uh... merge away, regret it later think some more.

@foolip
Copy link
Contributor

foolip commented Jun 18, 2021

OK, so final deal. I sent #11145 and think we should go with the 6.1→7 rewrite in #9423 (comment).

Can you back out all the 7→6.1 changes here?

@foolip
Copy link
Contributor

foolip commented Jun 19, 2021

This all also matches the api.WindowOrWorkerGlobalScope.indexedDB.worker_support entry:

"safari": {
"version_added": "10"
},
"safari_ios": {
"version_added": "10"
},

@foolip
Copy link
Contributor

foolip commented Jun 22, 2021

@vinyldarkscratch this is now only blocked on backing out some 10.1 / 10.3 versions that seems complicated.

@foolip foolip merged commit 37d667b into mdn:main Jun 22, 2021
@queengooborg queengooborg deleted the api/IDB/safari branch June 22, 2021 13:40
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.

2 participants