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_version to the safari_ios browser data #11299

Closed
wants to merge 4 commits into from

Conversation

foolip
Copy link
Collaborator

@foolip foolip commented Jun 29, 2021

The mapping for versions before 6 is complicated and can't be inferred
in a straightforward way from the existing data such as WebKit version.

This is based on research spread across issues and collected here:
https://gist.github.com/foolip/6872cf5350dccf6224fce984fb73fba1

@foolip foolip requested a review from ddbeck as a code owner June 29, 2021 10:56
@foolip foolip changed the title Add a safari_version to the safari_ios browser data Add safari_version to the safari_ios browser data Jun 29, 2021
@github-actions github-actions bot added data:browsers 🌍 Data about browsers (versions, release dates, etc). This data is used for validation. docs ✍️ Issues or pull requests regarding the documentation of this project. schema ⚙️ Isses or pull requests regarding the JSON schema files used in this project. labels Jun 29, 2021
@foolip
Copy link
Collaborator Author

foolip commented Jun 29, 2021

@ddbeck since this is a schema change, would it require a major release of BCD? I've filled out the whole mapping now and am pretty confident in it.

As you can see, there's a mapping from iOS→Safari, but not an unambiguous Safari→iOS mapping, which will make for fun times in the mirroring script. (Which currently just gets things wrong in some cases.)

@foolip foolip requested a review from queengooborg June 30, 2021 07:32
Copy link
Collaborator

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

Thanks for digging into this and finding all of the matching Safari releases! This is looking great to me, and I'll be happy to follow up with an update to the mirroring script to utilize these mappings!

Regarding the semver, I'd say this would only need a minor bump, since we're only adding a new feature and not modifying the existing features, especially in a backwards-compatibility-breaking way.

@ddbeck
Copy link
Collaborator

ddbeck commented Jul 1, 2021

Traditionally, we've done schema changes in semver major releases. This is an addition, so I'm not sure that's strictly required. That said, we're likely to do such a release pretty soon anyway.

A couple of questions/ideas of my own:

  • Did you consider replacing the values of engine and engine version instead? If you rejected that idea, why? Is there some benefit to having both engine and Safari version (for mirroring or any other purpose)?
  • I can't recall us doing anything overtly vendor or browser specific with the schema before. If it's important to have this data alongside the engine data, would you be open to reworking this so it's not vendor specific?

@foolip
Copy link
Collaborator Author

foolip commented Jul 5, 2021

Did you consider replacing the values of engine and engine version instead? If you rejected that idea, why? Is there some benefit to having both engine and Safari version (for mirroring or any other purpose)?

The trouble is that the engine versions don't align, Safari hasn't been released in lockstep for both desktop and iOS. The situation around Safari 4 is especially unusual.

I can't recall us doing anything overtly vendor or browser specific with the schema before. If it's important to have this data alongside the engine data, would you be open to reworking this so it's not vendor specific?

For all other browsers the mapping can be inferred from the engine. If there was another browser where that didn't work I would maybe suggest "matching_release": ["safari", "14.1"]. Does that seem better, even if only used for Safari iOS?

@Elchi3
Copy link
Member

Elchi3 commented Jul 13, 2021

This seems like something that could live in the mirror script internally. Can you comment on why we should make it part of the public BCD data (and thus guaranteeing to consumers that this data makes sense).

What is the definition of a "matching release"? (for Safari and generally)

@foolip
Copy link
Collaborator Author

foolip commented Jul 13, 2021

This seems like something that could live in the mirror script internally. Can you comment on why we should make it part of the public BCD data (and thus guaranteeing to consumers that this data makes sense).

Understanding the relationship between Safari and iOS releases is a lot of work, and it would be great to have it somewhat discoverable. It's not important that it's part of BCD releases, but I think it is important that a test fails if any entry is added to to safari_ios.json for which there is no known Safari mapping, since that would then break the mirror script.

What is the definition of a "matching release"? (for Safari and generally)

Two releases based on branches of the engine repo that branched from trunk at the same commit or pretty close, such that you can expect the same behavior from everything except stuff behind flags or changes made only on the release branches.

@Elchi3
Copy link
Member

Elchi3 commented Jul 13, 2021

Thanks for elaborating! I'm voting to land matching releases in test code and only make it public if it has proven to be generally useful for (external) consumers.

For Safari specifically, it would be good to get vendor feedback if the data makes sense / is correct.

@foolip
Copy link
Collaborator Author

foolip commented Jul 13, 2021

@gsnedders are you able to confirm if I got the mapping right here?

@foolip foolip marked this pull request as draft July 23, 2021 23:28
@gsnedders
Copy link
Contributor

The trouble is that the engine versions don't align, Safari hasn't been released in lockstep for both desktop and iOS. The situation around Safari 4 is especially unusual.

Can you expand on this? What happened, and what branches did those releases come from?

The mapping for versions before 6 is complicated and can't be inferred
in a straightforward way from the existing data such as WebKit version.

This is based on research spread across issues and collected here:
https://gist.github.com/foolip/6872cf5350dccf6224fce984fb73fba1
@queengooborg queengooborg added the semver-minor-bump ➕ A change that adds a new, non-potentially-breaking feature for consumers label Apr 22, 2022
@queengooborg
Copy link
Collaborator

Coming back to this, could we generalize this by setting the name to upstream_version?

@foolip
Copy link
Collaborator Author

foolip commented Jun 1, 2022

That would work, but @Elchi3 was very skeptical of adding anything about this to the data we expose to consumers and wanted to keep it in the mirroring script.

Could we maintain an exhaustive mapping in the mirroring script, as well as tests that check that inferring the mapping from engine version still works in all cases except the known exceptions?

@queengooborg
Copy link
Collaborator

I think that this data can be helpful to consumers, and that we should expose it -- not so much for Safari iOS, but for Samsung Internet, and Opera Android. I'd like us to keep zero data in the scripts, because I feel that if it's helpful data to us, it could very well be helpful to others.

@foolip
Copy link
Collaborator Author

foolip commented Jun 2, 2022

I agree that it might be helpful to consumers, but previous BCD owners have taken a much more conservative approach, not committing to a schema addition until there's a clear need. I assume the same argument would be made for #16393 too.

I think a lot of the mapping can be derived by code, can we try that instead of adding upstream_version to every release?

@queengooborg
Copy link
Collaborator

I agree that a schema change shouldn't be made without clear need, but personally, I see "change" and "addition" as separate things. Sure, we are modifying the schema files either way, but I feel that adding something in is less of an issue than altering existing behavior. Additionally, I feel that this is illustrated by a clear need; using engine versions for mapping isn't good enough for some browsers (Safari).

@foolip
Copy link
Collaborator Author

foolip commented Jun 5, 2022

I would support the change, but let's make sure it gets sign off from at least one other BCD owner.

@queengooborg
Copy link
Collaborator

@Rumyra When you're back in the office, may we get your opinion on this?

@foolip
Copy link
Collaborator Author

foolip commented Jun 5, 2022

One option we could consider is putting it in the individual browser JSON files but removing it from the built data, so that consumers can't depend on it.

@Rumyra
Copy link
Collaborator

Rumyra commented Jun 9, 2022

OK so just catching up... the issues is the mapping of Safari Desktop to Safari ios - which isn't historically consistent... this then affects mirroring.

(If I've understood) then thoughts would be:

  • If there's no data in the mirroring script then I would be inclined to keep it that way
  • Do we have any indication whether this data would be useful to consumers?
  • I quite like @foolip 's last comment as a compromise - to keep the data in the browser JSON but not expose it (that would depend on my previous question tho)

@foolip
Copy link
Collaborator Author

foolip commented Jun 9, 2022

If there's no data in the mirroring script then I would be inclined to keep it that way

This is basically true. There aren't any data tables, but there some mapping fixes like this one:

if (Number(version) <= 18) {
return '1';
} else if (Number(version) > 18 && Number(version) < 30) {
return '≤37';
} else if (Number(version) >= 30 && Number(version) < 33) {
return '4.4';
} else if (Number(version) >= 33 && Number(version) < 37) {
return '4.4.3';
} else {
return version;
}
};

That could also be replaced by the proposed upstream_version I think.

@github-actions
Copy link

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot added the merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. label Mar 3, 2023
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@queengooborg
Copy link
Collaborator

I just closed my similar PR that adds a more generic upstream_version property due to low interest. I'm thinking that we should close this one as well.

@Elchi3 Elchi3 closed this Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:browsers 🌍 Data about browsers (versions, release dates, etc). This data is used for validation. docs ✍️ Issues or pull requests regarding the documentation of this project. merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. schema ⚙️ Isses or pull requests regarding the JSON schema files used in this project. semver-minor-bump ➕ A change that adds a new, non-potentially-breaking feature for consumers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants