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

Linter: Make sure multiple version ranges are sorted according to some rules #1596

Closed
Elchi3 opened this issue Mar 18, 2018 · 5 comments · Fixed by #5352
Closed

Linter: Make sure multiple version ranges are sorted according to some rules #1596

Elchi3 opened this issue Mar 18, 2018 · 5 comments · Fixed by #5352
Assignees
Labels
bulk_update 📦 An update to a mass amount of data, or scripts/linters related to such changes HackOnMDNParis2018 🇫🇷 Issues or pull requests at the Hack on MDN event in Paris in March 2018 linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files.

Comments

@Elchi3
Copy link
Member

Elchi3 commented Mar 18, 2018

Given multiple version ranges ...

"webview_android": [
  {
    "version_added": "44"
   },
   {
    "alternative_name": "AnimationPlayer",
    "version_added": "39",
    "version_removed": "44"
  }
],

we should make sure that the first array element is always the most relevant (as this one will appear in the main compat cell).

For that we need to come up with some rules to guarantee "most relevant" first and generally make sense of the ordering there. Thoughts:

  • Order reverse-chronological
  • if there are prefixes, preferences, alternative_names or the like, rank less high.
@Elchi3 Elchi3 added linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. HackOnMDNParis2018 🇫🇷 Issues or pull requests at the Hack on MDN event in Paris in March 2018 labels Mar 18, 2018
@connorshea
Copy link
Contributor

I opened #1996 earlier with this same intent after finding a problem with the ServiceWorker page:

On the ServiceWorker page, this happens:

screen shot 2018-05-08 at 2 45 48 pm

The problem here is that the support array first has the information about Edge 16, where the feature is supported behind a flag, and then subsequently has information about Edge 17, where it's fully supported by default.

The ServiceWorker.json file has these lines:

"edge": [
{
"version_added": "16",
"flags": [
{
"type": "preference",
"name": "Enable service workers"
}
]
},
{
"version_added": "17"
}
],

The reason this is a problem is because Kumascript assumes that the support array will have the proper support info first, which in this case doesn't happen.

See these lines in kumascript:

// the first entry should be the most relevant/recent and will be treated as "the truth"
checkSupport(supportInfo[0].version_added, supportInfo[0].version_removed);

If we make this assumption, we should try to enforce it with the linter somehow, though I'm not sure how you'd go about doing that. 🤔

Here's an example where it works properly

From the flex page

screen shot 2018-05-08 at 2 53 00 pm

"firefox": [
{
"version_added": "20",
"notes": [
"Since Firefox 28, multi-line flexbox is supported.",
"Before Firefox 32, Firefox wasn't able to animate values starting or stopping at <code>0</code>."
]
},
{
"prefix": "-webkit-",
"version_added": "49"
},
{
"prefix": "-webkit-",
"version_added": "44",
"flags": [
{
"type": "preference",
"name": "layout.css.prefixes.webkit",
"value_to_set": "true"
}
]
},
{
"version_added": "18",
"version_removed": "28",
"flags": [
{
"type": "preference",
"name": "layout.css.flexbox.enabled",
"value_to_set": "true"
}
]
}
],

@Elchi3 Elchi3 added this to Linter improvements in Non-data issue overview Jan 10, 2019
@queengooborg queengooborg self-assigned this Dec 18, 2019
@queengooborg queengooborg added the bulk_update 📦 An update to a mass amount of data, or scripts/linters related to such changes label Dec 18, 2019
@ddbeck ddbeck added this to Tooling and broad data quality improvements in Prioritization review Sep 4, 2020
@caugner
Copy link
Contributor

caugner commented May 13, 2022

As concerns display of BCD on MDN, this has meanwhile been addressed by mdn/yari#5649 (support history) and mdn/yari#5946 (support cell).

/cc @queengooborg

@caugner
Copy link
Contributor

caugner commented May 13, 2022

FWIW This ranking/ordering algorithm implemented in yari is even a bit more sophisticated than the one suggested in #5352.

https://github.com/danielhjacobs/yari/blob/0e9484e8c432a7fbc0f3780b98655279d8c71182/client/src/document/ingredients/browser-compatibility-table/utils.ts#L147-L189


But ... I also wonder if it wouldn't be best to distinguish between repository and (npm) package.

In particular, always ordering items in the repository simply by their lowest version_number (i.e. item.version_added ?? 0), and then optionally by other criteria, has the big advantage that the position of each element is stable, and doesn't change as a side-effect of introducing a newer item (which is good for git blame).

In packaged BCD, the most relevant item could be moved to the top. Alternative approaches for the packaged BCD would be:

  1. Marking the current item with current: true.
  2. Adding a __current key to the feature that replicates the most relevant item.
  3. Restructuring the release data completely by separating the history (what BCD currently exposes) from a summarized "status" of that history (per browser, of course), or returning that on-demand:
type SupportStatusSummary =
  | { implemented: "never" }
  | {
      implemented: "partial" | "full"
      alternative_name?: string
      flags?: Array<{type: string, name: string, value_to_set: string}>
      version_added: string,
      version_removed?: string
    }

@queengooborg
Copy link
Collaborator

I personally feel that we should keep the contributor version as close to the bundled version as possible, and that this shouldn't introduce a schema-changing update. However, I'd be down to implement a sorting algorithm based on Yari's own, to synchronize the rendering on Yari with what other consumers will receive!

Prioritization review automation moved this from Tooling and broad data quality improvements to Done May 31, 2022
@queengooborg
Copy link
Collaborator

I've merged the PRs for migration 005, with the intention that we can follow-up with any changes to sorting in the later, should we desire to do so. (Not that it matters as much now, since Yari has their own sorting.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bulk_update 📦 An update to a mass amount of data, or scripts/linters related to such changes HackOnMDNParis2018 🇫🇷 Issues or pull requests at the Hack on MDN event in Paris in March 2018 linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files.
Projects
No open projects
5 participants
@Elchi3 @caugner @connorshea @queengooborg and others