-
Notifications
You must be signed in to change notification settings - Fork 174
Add Baseline regression notes #3187
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
34f003a
89c742b
fccb850
241e4e5
a96e279
f632460
e877da5
7fb07e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,6 @@ name: Streams | |
description: The streams API creates, composes, and consumes continuously generated data. | ||
spec: https://streams.spec.whatwg.org/ | ||
group: streams | ||
# TODO: https://github.com/web-platform-dx/web-features/issues/1971 | ||
# Status changed: https://github.com/web-platform-dx/web-features/pull/2358, https://github.com/web-platform-dx/web-features/pull/2491 | ||
# 2024-12-19 — low → false — Regressed status to match Caniuse, which considers support beginning at BYOB shipping. | ||
# 2025-01-30 — false → high — Split BYOB into a separate "readable-byte-streams" feature. Linked that one to Caniuse. | ||
# References: | ||
# - https://caniuse.com/streams | ||
Comment on lines
-5
to
-10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fussed over this for a while and ended up deciding to omit the note entirely. The first note would say that the feature regressed, the second note would be some new category (advance? unregression?) showing that we more or less reverted the second note. It seemed to me that the correct course of action in that scenario would be to withdraw the note, so that's what I've done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to agree. If our consumers really only care about baseline regressions, then we don't need a note here. Unless we have reasons to believe that the history of a feature would be useful. Which I don't think we do. |
||
status: | ||
compute_from: | ||
- api.ReadableStream | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -134,6 +134,13 @@ for (const [key, data] of yamlEntries('features')) { | |
data.description = text; | ||
data.description_html = html; | ||
} | ||
if (Array.isArray(data.notes)) { | ||
for (const note of data.notes as FeatureData["notes"]) { | ||
const { text, html } = convertMarkdown(data.description); | ||
note.message = text; | ||
note.message_html = html; | ||
} | ||
} | ||
|
||
// Compute Baseline high date from low date. | ||
if (data.status?.baseline === 'high') { | ||
|
@@ -163,6 +170,15 @@ for (const [key, data] of yamlEntries('features')) { | |
} | ||
} | ||
|
||
// Ensure regression notes are still relevant | ||
if (Array.isArray(data.notes)) { | ||
for (const [index, note] of (data.notes as FeatureData["notes"]).entries()) { | ||
if (note.new_baseline_value !== data.status.baseline) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah cool! I see you've actually already done what I suggested in an earlier comment. |
||
throw new Error(`regression note ${index} on ${key}.yml no longer applies (status is ${data.status.baseline}, note is ${note.new_baseline_value}). Delete this note.`); | ||
} | ||
} | ||
} | ||
|
||
if (data.compat_features) { | ||
// Sort compat_features so that grouping and ordering in dist files has | ||
// no effect on what web-features users see. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,8 @@ export interface FeatureData { | |
compat_features?: string[]; | ||
/** Whether developers are formally discouraged from using this feature */ | ||
discouraged?: Discouraged; | ||
/** Notes about this feature */ | ||
notes?: BaselineRegressionNote[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need this to be an array? I don't think we do, but I can see the point that not making it an array might, some day, block us, and require a major release to change it then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, @captainbrosset! I think it's very likely that we'll have other note types and notes may need to coexist. My inclination was to introduce one note type, then add more. But I recognize it looks a little odd now because there really ought to be just one note at a time under the constraints presented here. But there's some likely follow up note types: First, consider the mirror of a regression: a case where we've prevented a feature from advancing (think of it as Second, there are cases where we might choose to not regress a feature, like #2957 (though it's still my preference to let it regress). It seems to me it might be wise for us to (publicly) acknowledge the bug, even if we do not believe it enough to regress the feature (something like I imagine there are still further note types (e.g., something like solicitations for input on potentially deprecating or removing a feature, for instance, as in the case of XSLT), but I haven't given them a ton of thought yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. The |
||
} | ||
|
||
type BrowserIdentifier = "chrome" | "chrome_android" | "edge" | "firefox" | "firefox_android" | "safari" | "safari_ios"; | ||
|
@@ -82,6 +84,27 @@ interface Discouraged { | |
// https://github.com/web-platform-dx/web-features/issues/2722 is resolved. | ||
} | ||
|
||
/** | ||
* A note describing a Baseline status regression. | ||
* For example, a feature that has moved from Baseline low to not Baseline. | ||
*/ | ||
interface BaselineRegressionNote { | ||
/** The topic of this note. This field is also a discriminator for any future note types */ | ||
category: "baseline-regression"; | ||
/** The date that the regression was added to web-features data */ | ||
date: string; | ||
/** A short description of the cause of the regression as a plain text */ | ||
message: string; | ||
/** A short description of the cause of the regression as HTML */ | ||
message_html: string; | ||
/** One or more URLs, such as bugs, used to justify the regression */ | ||
citations: string[]; | ||
/** The `baseline` status value before the regression */ | ||
old_baseline_value: "high" | "low"; | ||
/** The `baseline` status value after the regression */ | ||
new_baseline_value: "low" | false; | ||
} | ||
|
||
export interface GroupData { | ||
/** Short name */ | ||
name: string; | ||
|
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.
When I opened this PR, I had written this note. However, the feature has since progressed. It occurred to me that we should not show irrelevant notes, so I commented it out. This is a rather new idea, so I'd like to delete this entirely, if we decide this is the right way to go.
Other options considered: some sort of "historic" flag on notes that are no longer relevant, or filtering historic notes from the published package. I figured both of those added complexity that wouldn't be very useful to anyone, least of all developers.
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 almost feel like we should have a linting step in place which checks that
notes.new_baseline_value
is equal tostatus.baseline
, and if not, deletes that particular note. I think keeping old notes about status regressions that no longer apply would be confusing. I don't think that web-features necessarily needs to keep track of the history of a feature. Its role is to document the platform as it is now.