browser-compat-data icon indicating copy to clipboard operation
browser-compat-data copied to clipboard

Add safari_version to the safari_ios browser data

Open foolip opened this issue 3 years ago • 19 comments

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 avatar Jun 29 '21 10:06 foolip

@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 avatar Jun 29 '21 11:06 foolip

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?

ddbeck avatar Jul 01 '21 15:07 ddbeck

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?

foolip avatar Jul 05 '21 13:07 foolip

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)

Elchi3 avatar Jul 13 '21 13:07 Elchi3

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.

foolip avatar Jul 13 '21 13:07 foolip

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.

Elchi3 avatar Jul 13 '21 13:07 Elchi3

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

foolip avatar Jul 13 '21 14:07 foolip

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?

gsnedders avatar Jul 26 '21 13:07 gsnedders

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

queengooborg avatar Jun 01 '22 17:06 queengooborg

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?

foolip avatar Jun 01 '22 21:06 foolip

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.

queengooborg avatar Jun 01 '22 22:06 queengooborg

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 https://github.com/mdn/browser-compat-data/pull/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?

foolip avatar Jun 02 '22 09:06 foolip

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

queengooborg avatar Jun 05 '22 07:06 queengooborg

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

foolip avatar Jun 05 '22 10:06 foolip

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

queengooborg avatar Jun 05 '22 11:06 queengooborg

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.

foolip avatar Jun 05 '22 21:06 foolip

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)

Rumyra avatar Jun 09 '22 10:06 Rumyra

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:

https://github.com/mdn/browser-compat-data/blob/1147dbfc1e390ab201ef0429be0ee0e03f5ebc53/scripts/release/mirror.js#L280-L291

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

foolip avatar Jun 09 '22 15:06 foolip

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

github-actions[bot] avatar Jul 30 '22 20:07 github-actions[bot]

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

github-actions[bot] avatar Mar 03 '23 12:03 github-actions[bot]

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.

queengooborg avatar Nov 06 '23 11:11 queengooborg