-
Notifications
You must be signed in to change notification settings - Fork 75
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
Provide an approved method to get a status for a single BCD key #1204
Provide an approved method to get a status for a single BCD key #1204
Conversation
Note that this probably needs owner's group consensus before merging. |
1692cc8
to
ca8bd79
Compare
See also: #1173 (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.
I'm generally in favor of this approach. Just left a few comments.
packages/compute-baseline/README.md
Outdated
}, | ||
compat, | ||
); | ||
getStatus("fetch", "api.Response.json", compat); |
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.
Can you help me understand when someone would need to use their own compat data? Are there other data sources out there that match the BCD schema and which getStatus
would be able to work with?
If this is a valid use case, then I would suggest not using the getStatus
method for it, since getStatus
is for blessed/editorial reviewed Baseline statuses only. But if you use your own data, then we can't guaranty anything about 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 this is for MDN to make sure that they use the same BCD for Baseline status and the compat tables. I'm not sure what their setup is though, it could be that they get this automatically simply by having the project (yari) depend on one version of BCD.
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, I was trying to accommodate (for example) MDN upgrading BCD independently of web-features. Really, I'd like to be able to say something like: "You can use any version of BCD >= web-features's version of BCD" but we don't currently provide that information to consumers of web-features.
Another route here is to leave this usage undocumented.
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.
Upon further consideration, I've decided to undo this change in the example. Really, consumers ought to use the latest BCD unless/until we can do some more clever enforcement.
// TODO: actually check that featureId is a valid feature | ||
// TODO: actually check that compatKey is tagged as featureId in BCD _or_ listed in web-features |
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.
Are you planning on implementing those TODOs in this PR?
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 am not. This requires some non-trivial changes to the way the repo and dependencies are set up, which would overwhelm the policy and documentation changes going on here.
You can use `compute-baseline` to explore possibilities or do error correction. | ||
For example, you might use `compute-baseline` to hide a Baseline status, when showing a broader feature's status might be misleading. | ||
If you need to know the Baseline status of a specific browser compatibility data entry within a `web-features` feature, then you can use the `getStatus` method. | ||
The `web-features` package and the `getStatus` method are the **only** ways to get a status that have completed the full Baseline editorial review process. |
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.
In order for this to be accurate, we need a way to review the individual statuses. Grouping compat keys by their generated status with comments is the approach we've discussed.
packages/compute-baseline/README.md
Outdated
}, | ||
compat, | ||
); | ||
getStatus("fetch", "api.Response.json", compat); |
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 this is for MDN to make sure that they use the same BCD for Baseline status and the compat tables. I'm not sure what their setup is though, it could be that they get this automatically simply by having the project (yari) depend on one version of BCD.
// TODO: actually check that compatKey is tagged as featureId in BCD _or_ listed in web-features | ||
return JSON.parse( | ||
computeBaseline( | ||
{ compatKeys: [compatKey], checkAncestors: 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.
Can you add a test that shows the difference between checkAncestors
being true or false? Is it about flags that aren't correctly cascaded?
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.
This is already tested with
web-features/packages/compute-baseline/src/baseline/index.test.ts
Lines 78 to 104 in cc5cccb
it("finds discrepancies with ancestors (checkAncestors)", function () { | |
const result = computeBaseline({ | |
compatKeys: ["api.Notification.body"], | |
checkAncestors: false, | |
}); | |
const resultExplicit = computeBaseline({ | |
compatKeys: ["api.Notification", "api.Notification.body"], | |
checkAncestors: false, | |
}); | |
const resultWithAncestors = computeBaseline({ | |
compatKeys: ["api.Notification.body"], | |
checkAncestors: true, | |
}); | |
assert.equal(resultExplicit.toJSON(), resultWithAncestors.toJSON()); | |
assert.notEqual(result.toJSON(), resultWithAncestors.toJSON()); | |
assert.notEqual(result.baseline, resultWithAncestors.baseline); | |
assert.notEqual( | |
result.baseline_low_date?.toString(), | |
resultWithAncestors.baseline_low_date?.toString(), | |
); | |
chai.expect(result).to.matchSnapshot(); | |
chai.expect(resultExplicit).to.matchSnapshot(); | |
chai.expect(resultWithAncestors).to.matchSnapshot(); | |
}); |
getStatus
does the same?
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.
Ah, existing tests are OK.
Co-authored-by: Patrick Brosset <patrickbrosset@gmail.com>
Hi folks, I tagged the current owners for review. I brought this PR up on the last WebDX call (and the idea in general in previous web-features/Baseline calls), since it's a little bit of a departure from how we've done editorial review so far. I suspect this is getting close to landing, so if any of the owners want to personally review it before we move forward, I wanted to make sure there was an explicit opportunity for that. Thank you! |
featureId: string, | ||
compatKey: string, | ||
compat: Compat = defaultCompat, | ||
) { |
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.
Can you add the return type here? It looks like it's not the same as computeBaseline
, and I wonder if that's intentional.
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.
Done with 63e00ad. (Sorting out the peer dependency between compute-baseline
and web-features
will be fussy. Saving it for a PR just for that.)
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.
LGTM!
Thanks to everyone who had a look at this (or listened to me ramble on about it in the WebDX call last week)! |
This adds an approved method, called
getStatus()
, to turn aweb-features
ID and a BCD key into a status for that specific BCD key.Right now, this method is a formalism: it doesn't actually check that the BCD key is part of the named feature (or that the named feature even exists). But this illustrates the overall principle and will let us actually clamp down in follow up PRs, if this is the right approach.
Towards #979.