compute-baseline: report version ranges and ranged dates
#1398
Conversation
There was a problem hiding this comment.
I had hoped that we can avoid ranges in Baseline dates, but from #1399 it's clear we'll have a lot of them, and that a lot of dates will depend on which version of Edge 12-79 is correct.
Researching those Edge release versions is going to be a lot of work and I don't think we can block web-features on it. But I hope we can minimize ranges dates for the feature itself.
| @@ -140,7 +173,7 @@ describe("computeBaseline", function () { | |||
| describe("keystoneDateToStatus()", function () { | |||
| it('returns "low" for date 1 year before cutoff date', function () { | |||
| const status = keystoneDateToStatus( | |||
| Temporal.PlainDate.from("2020-01-01"), | |||
| Temporal.PlainDate.from("2020-01-01").toString(), | |||
There was a problem hiding this comment.
Simplify?
| Temporal.PlainDate.from("2020-01-01").toString(), | |
| "2020-01-01", |
Also, can you add a test for keystoneDateToStatus where a ranged date is used, to check that it's preserved?
| const [date, ranged] = dateSpec | ||
| ? parseRangedDateString(dateSpec) | ||
| : [null, false]; | ||
|
|
||
| if (date == null || discouraged) { |
There was a problem hiding this comment.
| if (date == null || discouraged) { | |
| if (dateSpec == null || discouraged) { |
| cutoffDate: Temporal.PlainDate, | ||
| discouraged: boolean, | ||
| ): { | ||
| baseline: BaselineStatus; | ||
| baseline_low_date: BaselineDate; | ||
| baseline_high_date: BaselineDate; | ||
| } { | ||
| const [date, ranged] = dateSpec |
There was a problem hiding this comment.
Can you put this after the early return just below so that we know that dateSpec it not null?
| } | ||
| return latestDate; | ||
| return `${keystone.release.date}`; |
There was a problem hiding this comment.
| return `${keystone.release.date}`; | |
| return keystone.release.date; |
There was a problem hiding this comment.
keystone.release.date is a Temporal.PlainDate object, not a string. Based on your suggestion, I assume that this isn't obvious, so I've turned this into return keystone.release.date.toString() with d2723c7.
| if (!lastInitial) { | ||
| if ([false, null].includes(current)) { | ||
| // First-iteration only: bail when the latest release does not support | ||
| // the feature. Strictly speaking, this could be a future release |
There was a problem hiding this comment.
Is this comment really true? We start from b.current.releaseIndex and move backwards, how could we find a future release?
There was a problem hiding this comment.
No, it's a stale text—it originally iterated from the latest release (including preview) but that also surfaced new values (e.g., "preview" or future versions) in an unhelpful/unclear way. I've removed the incorrect text with 8eada1e.
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
…orm-dx#1398) Co-authored-by: Philip Jägenstedt <philip@foolip.org>
You can see what this would look like in #1399