-
Notifications
You must be signed in to change notification settings - Fork 173
Add move and split redirects to the schema #3000
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: v3.0
Are you sure you want to change the base?
Conversation
features/webcodecs.yml
Outdated
name: WebCodecs | ||
description: The WebCodecs API provides low-level access to individual video frames and chunks of audio samples, for full control over the way media is processed. | ||
spec: https://w3c.github.io/webcodecs/ | ||
caniuse: webcodecs |
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 we should split it, but the fact that this is in caniuse makes me think we should somehow keep webcodecs as well? With composite features or similar we could.
Do we have any other candidates for splitting where the existing entry is just wrong and has no value to preserve?
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 a good point. One thing I haven't done is to write guidelines that say when to move or split a feature—I just put these in to demonstrate the fact that one could do it. I think it would actually be best to back out the YAML files from this PR before merging and land those changes separately, especially to make the release notes more informative.
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.
Which is to say, once I've written the guidelines, webcodecs might not get a split at all.
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, making the actual changes separately sounds good!
We do need to make sure we have a real case for splitting however. If we can only come up with "composite" features that we might later want to merge back together, then I'm not sure we should remove the original feature. Do have features that were combined by mistake and should never be considered one?
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.
Hmm, I'm not currently seeing a compulsory split, if we're assuming some future composite feature schema.
It's somewhat easier to find historic cases. #1089 is a case where, without a splitting schema, would produce either a) a breaking release (which is basically what we did, in the pre-1.0 era) or b) a useless composite feature that combines something interoperable and something that never will be. In the latter case, caniuse has a corresponding feature, but I don't think we should follow caniuse's lead there.
There are also cases we've already merged that look a little suspect, where we split something by moving keys, tweaking a description, and just kinda pretending we got it right in the first place (e.g., #2491, #2652). I don't wish to bank on always being on the right side of that line.
Which brings me to my last concern: split
ought to be the reverse of merged
. If we wish to be able to merge freely, then we'll need a safe undo. Otherwise, merges will have the feeling of a semver-major event for maintainers here, even if it doesn't bump the version number.
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 use #2776 as the feature to demonstrating split. We'd split that into gradients (2 BCD keys) and conic-gradients (1 BCD key).
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 switched to the single color stop feature as our demonstrator in 9b276f0
@jcscottiii I've added the But, once I added the discriminator field, I found that the existing structure was duplicative. Using the discriminator meant I could flatten the feature data—this greatly simplifies distinguishing the types (even if Quicktype ignores this fact). We now have things like this: { "kind": "feature", "name": "<abbr>", "description": "…" }
{ "kind": "moved", "redirect_target": "…" }
{ "kind": "split", "redirect_targets": ["feat1", "feat2"] } I'm curious to know your thoughts on this variation. |
The actual schema validation is failing right now (but strangely, only for the split feature). Trying to figure out why exactly. Edit: it was a bug in the script that assembles the |
Ahh. Thanks for pointing out that the discriminator is an OpenAPI-only feature. I like the use of I will pull in those changes tomorrow in my PR and give an update. |
I updated my PR and it looks to me. The |
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 have not reviewed everything else. But from the data.schema.json POV, it looks good to me! I only have one nit/question.
}, | ||
"minItems": 1 | ||
}, | ||
"URLOrURLs": { |
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.
Nit: Currently this is the same as StringOrStrings. I think you meant to add the format
of the string? Otherwise, could you simplify to use StringOrStrings.
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.
Good catch. I'll fix this more comprehensively with #3184.
Question 1I have a question about the cases when a feature is split up partially (i.e. the scope of feature is narrowed). For a feature that is partially split (meaning its scope is narrowed, and new features are spun off while the original
Question 2Talking about timestamps, do we want to include a date for the move in the schema too? For both questions, I'm okay keeping it as-is. I just started to think of different possibilities when I was working on GoogleChrome/webstatus.dev#1677 |
One more question about split features: For those features that are partially split out, how will the YAML look with this 3.0.0 schema? When I say partially split out, I mean Feature A still exists but it has been scoped down and now feature B and C are split out of Feature A. I was writing some e2e tests for webstatus.dev and currently I don't see a way for Feature A to use both |
Now targeting the v3.0 branch (which was created from main a minute ago, so the conflicts here are the same as they were before). |
@jcscottiii Good question! The main answer is: partial splits can't happen. In some cases, we'll error correct a feature silently (e.g., if a few compat keys were assigned erroneously—that wouldn't impact the meaning of the name, description, mapping to caniuse, etc.—then we'll just move those keys to another feature)—we do this already. Otherwise, the feature will have to be fully split (i.e., the original ID is just a pointer to two or more others). I've given some thoughts to more sophistication around partial splits, but it's something we'd have to consider in a v4.0.0 (along with a notion of aspects of a larger feature and/or composite features). |
Sounds good to me! Background: I was just thinking about adding a banner to the feature detail page on webstatus.dev to let users know that they could reference other features in the event the original feature's scope was downsized. But I couldn't see a way for the current schema to tell me that information directly. In the meantime, if we really wanted this, we could potentially track the BCD keys for each feature. But that introduces its own challenges, such as determining whether a key's removal was an editorial correction or a true feature split. Anyway, I'm completely okay leaving this for v4 once we have more usage of this. |
Currently, v3 cannot let us know if a feature has been partially split out. This commit removes the split out info from the Regular Feature model. See comment: web-platform-dx/web-features#3000 (comment) This allows us to simplify this. We can revisit in v4.
* fix: Return correct feature evolution data for moved and split features The primary changes are: - The `GetMovedWebFeatureDetailsByOriginalFeatureKey` function has been fixed. It was previously returning the internal database ID of the new feature; it now correctly returns the human-readable feature key, as intended. - The main feature query has been updated to join against a new `SplitWebFeatures` table. The `GET /v1/features` and `GET /v1/features/{feature_id}` endpoints now include an `evolution` object in the response. This object contains `split_off_info` which lists any features that have been split off from the primary feature. To support this, the following changes were also made: - The `openapi.yaml` specification has been updated to include the new `evolution` fields in the `Feature` schema. - The fake data generation process in `util/cmd/load_fake_data` has been significantly enhanced to create more realistic test data for feature evolution, including scenarios for moved, fully split, and partially split features. - End-to-end tests and snapshots have been updated to reflect the new data structures and the changes in the underlying test data.start There needs to be some frontend changes. But that will come in a future PR. * fix snapshots * Remove split out info from Regular Feature model Currently, v3 cannot let us know if a feature has been partially split out. This commit removes the split out info from the Regular Feature model. See comment: web-platform-dx/web-features#3000 (comment) This allows us to simplify this. We can revisit in v4. * update snapshots
Towards #91 and the sequel to #2990.
Because this PR follows #2990, looking at the diff between them might be easier to look at. See: ddbeck/web-features@91-jsonschema-source-of-truth...ddbeck:web-features:91-redirects
This PR does the following:
webcodecs
(Splitwebcodecs
intoaudio-codecs
andvideo-codecs
#2918) and movingnumeric-seperators
tonumeric-separators
(Fix typo in file name #2484)This PR mostly follows the mock documentation discussed from #91 (see #91 (comment)). Some changes I made based on the discussion there:
reason
for changing an ID ismoved
, instead ofrenamed
, to avoid implying that the ID will change any time the human-readablename
field changes.to
string/array of strings is now two distinct fields,target
andtargets
, depending on thereason
. This is to avoid some weirdness in using a preposition as a noun (i.e., to avoid saying silly things like "this feature has two tos") and to ensure the name of the field implies its type.