-
Notifications
You must be signed in to change notification settings - Fork 76
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
Allow use of ranges (≤) before the Baseline low date #1208
Allow use of ranges (≤) before the Baseline low date #1208
Conversation
7846d59
to
c0afc72
Compare
I have tried sprinkling this onto the HTML element draft features and really like what it's doing. For example: --- a/features/draft/html-elements/h1.yml.dist
+++ b/features/draft/html-elements/h1.yml.dist
@@ -11,5 +11,5 @@ status:
edge: "12"
firefox: "1"
firefox_android: "4"
- safari: "4"
- safari_ios: "3.2"
+ safari: ≤4
+ safari_ios: ≤3.2 Not having to spend any time updating BCD to show that |
index.ts
Outdated
if (data.status?.support) { | ||
const versions: string[] = Object.values(data.status.support); | ||
if (versions.some((v) => v.startsWith('≤'))) { | ||
if (data.status.baseline_low_date !== earliestBaselineLowDate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I change this to allow for ranges when the Baseline low date is before 2016? I'm on the fence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this being a more specific thing to start, then expanded to later uses later (as needed).
@@ -5,3 +5,4 @@ group: forms | |||
caniuse: input-search | |||
# TODO: investigate minor differences in the early support of the feature | |||
# between https://caniuse.com/input-search and our generated status. | |||
use_ranges_before_baseline_low_date: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only just occurred to me: should we consider indicating internal-only fields? For example, prefixing with an underscore? Doesn't have to happen in this PR, but it'd be nice to get your thoughts on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would provide a useful clue to someone reading the source and wondering if they can use the data through a published package. However, it wouldn't make it any easier for us to filter out such keys, because in order to catch typos we have to enumerate all allowed keys.
chrome: "5" | ||
chrome: ≤5 | ||
chrome_android: "18" | ||
edge: "12" | ||
firefox: "4" | ||
firefox: ≤4 | ||
firefox_android: "4" | ||
safari: "5" | ||
safari_ios: "4.2" | ||
safari: ≤5 | ||
safari_ios: ≤4.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm taking this to mean "Edge 12 is the very latest release to introduce support for this feature but other browsers may have come earlier than we report."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, which is different from "it may have come earlier or later than we report" which would be a natural outcome of "we're not sure we trust this data and haven't reviewed it carefully." But, when throwing together a bunch of things, the most recent version is the one we use, so there's good reason to expect that the error will usually be mostly in the direction of claiming features are newer than they are.
index.ts
Outdated
@@ -20,16 +20,20 @@ const nameMaxLength = 80; | |||
// The longest description allowed, to avoid them growing into documentation. | |||
const descriptionMaxLength = 300; | |||
|
|||
// The earliest possible Baseline low date, the release date of Edge 12. | |||
const earliestBaselineLowDate = '2015-07-29'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is better exactly, but it feels more "correct":
const earliestBaselineLowDate = '2015-07-29'; | |
// needs import { browser } from "compute-baseline/browser-compat-data"; | |
const earliestBaselineLowDate = browser("edge").version("12").date.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If taking one step I'd be tempted to take the next one, and compute from the core browser set what the earliest possible Baseline low date is.
But I think it would be just as good to just say that ranges are allowed for any pre-2016 data except the keystone release for each feature. Once it's 2026 this will be quite easy to justify too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, if this will eventually become a date, let's leave it as a date.
index.ts
Outdated
if (data.status?.support) { | ||
const versions: string[] = Object.values(data.status.support); | ||
if (versions.some((v) => v.startsWith('≤'))) { | ||
if (data.status.baseline_low_date !== earliestBaselineLowDate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this being a more specific thing to start, then expanded to later uses later (as needed).
// Ensure that version ranges are only used for prehistoric features. | ||
if (data.status?.support) { | ||
const versions: string[] = Object.values(data.status.support); | ||
if (versions.some((v) => v.startsWith('≤'))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love adding uncertainty to potentially already-certain ranges in BCD, but I expect a future resolution to #1211 to fix this at a later date, so it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the discomfort. Should we instead have a single possible combination of ranged browser versions as described in #1018 (comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave this for now too. I think bubbling up BCD's uncertainty will mess with a bunch of values anyway (especially dates), so let's sort that out first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, let me make some cosmetic changes around the Edge 12 date and land this.
3335b72
to
c58ed0f
Compare
@foolip I think this is ready to go, pending a main merge or rebase. |
347d270
to
dcc0587
Compare
The `use_ranges_before_baseline_low_date` field controls when generating the status, but it could also be done in overrides. Closes web-platform-dx#1018.
dcc0587
to
c09ce0b
Compare
This got in the way of grouping comments, see https://github.com/web-platform-dx/web-features/pull/1231/files#r1656826439. I'm considering if we should revert this as it's less important than #1231. |
The
use_ranges_before_baseline_low_date
field controls when generatingthe status, but it could also be done in overrides.
Closes #1018.