Skip to content
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

Add support for 'spec-urls' frontmatter key #5284

Merged
merged 1 commit into from
Feb 17, 2022
Merged

Add support for 'spec-urls' frontmatter key #5284

merged 1 commit into from
Feb 17, 2022

Conversation

sideshowbarker
Copy link
Member

@sideshowbarker sideshowbarker commented Feb 15, 2022

Fixes #4704 • Related: #5277

This change adds support for listing specs in the Specifications section based on the value of the spec-urls frontmatter key — instead of or in addition to listing specs based on the value of the browser-compat frontmatter key, or the value of the parameter passed to the Specifications macro.

Fixes #4704
Related: #5277

This change adds support for listing specs in the Specifications section
based on the value of the 'spec-urls' frontmatter key — instead of or in
addition to listing specs based on the value of the 'browser-compat'
frontmatter key, or the value of the parameter passed to the
Specifications macro.
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, Mike! Looks good to me 👍

@schalkneethling schalkneethling self-assigned this Feb 17, 2022
@schalkneethling schalkneethling added the enhancement Improves an existing feature. label Feb 17, 2022
Copy link

@schalkneethling schalkneethling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @sideshowbarker 🎉

@hamishwillee
Copy link
Contributor

@schalkneethling @sideshowbarker How does this work?

  1. What's the format for declaring a single spec or multiple specs? I.e. is is a copy of what you do in BCD?

    So for single URL

    spec_urls: https://drafts.csswg.org/css-inline/#alignment-baseline-property
    

    For multiple URLs:

    spec_urls: [
               "https://drafts.csswg.org/css-inline/#alignment-baseline-property",
               "https://svgwg.org/svg2-draft/text.html#AlignmentBaselineProperty"
             ],
    
  2. Do the spec urls still have to be valid to the same format as BCD (?) and in the associated spec definition data used by BCD?

  3. How does this key work with respect to the BCD key?

    • Are specs appended to BCD specs if both are specified? Or do they replace them?
    • If I specify a BCD key in {{Specifications("a key")}} does it still take the same info from the spec url key in metadata?

@sideshowbarker sideshowbarker deleted the sideshowbarker/Specifications-macro-spec-urls-frontmatter-key-support branch February 21, 2022 00:59
@sideshowbarker
Copy link
Member Author

How does this work?

I’ve started writing some documentation for it, and updating the templates. So as part of getting the initial spec URLs added (for the docs where we already have them, but in old-style manual tables, and so can automated the process of adding them), I plan to have the draft documentation ready for review. But since the documentation should be informed by real cases, it seems to make sense to first get the spec URLs added for the existing real cases that we have — so then the documentation can reflect whatever info/patterns we can notice from looking at those.

@wbamberg
Copy link
Collaborator

It looks like this is representing multiple spec URLs as a comma-separated string rather than an array of strings, e.g. https://github.com/mdn/content/pull/13198/files#diff-fc3c88bc0f74503f8b68c76ebff9648624ba3f0985d0d9f59b141f5ff7b4ada2R11-R13. Is there a reason we are doing it this way? It seems harder to read and harder to work with.

@sideshowbarker
Copy link
Member Author

Yeah, I agree it’s better to have a YAML array for the multiple-URLs cases. The YAML multi-line string thing is not ideal.

Sorry, when I wrote the code, I just hadn’t thought much about the multiple-URL case.

Anyway, at this point I’ll write up a patch for the code to make it output a YAML array for the multiple-URLs cases — as well as writing patches for the existing cases of multiple-URL articles sources that’ve already been merged.

@wbamberg
Copy link
Collaborator

Sorry to put you to the trouble Mike. But I do think it's worth addressing.

@sideshowbarker
Copy link
Member Author

Sorry to put you to the trouble Mike. But I do think it's worth addressing.

Definitely — thanks for catching it

@sideshowbarker
Copy link
Member Author

sideshowbarker commented Feb 23, 2022

It looks like this is representing multiple spec URLs as a comma-separated string rather than an array of strings

So as far as I can tell, this consuming code in Yari appears to work fine as-is if the spec-urls value in the YAML source is a YAML array rather than a string.

Given that, it seems like there’s nothing to change in this existing consumer code that’s already merged into Yari.

That said, over in #5309, I did need to change my (unmerged) modifications to the h2m CLI — to make it output a YAML array for the case where there are multiple spec URLs:

yari/markdown/cli.ts

Lines 244 to 246 in 26f5d2c

metadata["spec-urls"] =
// String, if only on spec URL; otherwise, array.
specURLs.length === 1 ? specURLs[0] : specURLs;

But, for common case where there’s only one spec URL, that code still outputs a YAML string on a single line, like this:

spec-urls: https://w3c.github.io/webcrypto/#dfn-AesCbcParams

And since — as mentioned above — the existing consuming code in Yari that’s already been merged works fine both when the spec-urls value in the frontmatter metadata is simple YAML string and when it’s a YAML array.

Given that, I think we should allow authors to use a string for the spec-urls value when there’s only one URL (and document it that way). That seems preferable from an authoring point of view — because otherwise, we’d be requiring authors to instead, every single time, use the YAML array syntax even if there’s only one spec URL; e.g.:

spec-urls:
  - https://w3c.github.io/webcrypto/#dfn-AesCbcParams

And even if we did require that, I can imagine that even when writing docs ourselves, we might sometimes forget that we’re supposed to use a YAML array there, and we’d just instead use a string. But the thing is, since that would still work, we wouldn’t even know we were violating the use-an-array rule — unless we were somehow linting for it.

In fact, given that the existing code also works as expected even in the case that somebody gives the spec-url key a comma-separated value, I guess it’s pretty robust as-is — as far as not being fragile under authoring errors.

Anyhow, that’s just a long-winded way of saying that I think we don’t need to change anything in the code, and we can just have the documentation say

If there’s only one spec URL, just put the URL on the same line as the spec-urls: key — but for multiple URLs, list them same way the tags values are listed: each URL on its own line, indented by two spaces and prefixed with "- ".

@teoli2003
Copy link
Contributor

Given that, I think we should allow authors to use a string for the spec-urls value when there’s only one URL (and document it that way).

I agree with this. Let's wait for Will's feedback and we can move again forward on the content side.

@wbamberg
Copy link
Collaborator

wbamberg commented Feb 23, 2022

Given that, I think we should allow authors to use a string for the spec-urls value when there’s only one URL (and document it that way).

I agree with this. Let's wait for Will's feedback and we can move again forward on the content side.

Yes, I think we should say spec-urls can be:

  • a string value when there is only one (the common case)
  • an array of strings when there is more than one

@hamishwillee had some interesting questions.

  1. What's the format for declaring a single spec or multiple specs? I.e. is is a copy of what you do in BCD?
    So for single URL
spec_urls: https://drafts.csswg.org/css-inline/#alignment-baseline-property  

For multiple URLs:

spec_urls: [
           "https://drafts.csswg.org/css-inline/#alignment-baseline-property",
           "https://svgwg.org/svg2-draft/text.html#AlignmentBaselineProperty"
         ],

Sort of, except this is YAML, so:

 spec_urls: 
    - https://drafts.csswg.org/css-inline/#alignment-baseline-property
    - https://svgwg.org/svg2-draft/text.html#AlignmentBaselineProperty
  1. Do the spec urls still have to be valid to the same format as BCD (?) and in the associated spec definition data used by BCD?

Ideally we would have the same validation for these spec URLs as we have for BCD ones. But I think that is not in place at the moment.

  1. How does this key work with respect to the BCD key?
  • Are specs appended to BCD specs if both are specified? Or do they replace them?

In the version implemented by this PR, it looks like if both BCD and spec-urls provide spec URLs, then they are all included. But as I had envisaged this, spec-urls is a fallback for when a page has no BCD to put the spec URL. So ideally, IMO:

if the browser-compat key is present at all, then spec-urls is ignored (or even better, is an error)

This makes things much clearer for authors.

  • If I specify a BCD key in {{Specifications("a key")}} does it still take the same info from the spec url key in metadata?

Great point, I had forgotten about that. IIRC the main function of that argument was a migration aid while we were adding browser-compat to front matter. Ideally I'd hope we could remove all usages of {{Specifications("some.bcd.query")}}, and then remove it from the macro.

Right now there are only a few instances of it:

Instances of "{{Specifications("a key")}}"
./files/en-us/web/web_components/index.md:{{Specifications("html.elements.template")}}
./files/en-us/web/web_components/index.md:{{Specifications("api.ShadowRoot")}}
./files/en-us/web/api/css_typed_om_api/index.md:{{Specifications("api.CSSStyleValue")}}
./files/en-us/web/api/console_api/index.md:{{Specifications("api.console")}}
./files/en-us/web/api/css_properties_and_values_api/index.md:{{Specifications("api.CSS.registerProperty")}}
./files/en-us/web/api/web_audio_api/index.md:{{Specifications("api.AudioContext")}}
./files/en-us/web/api/htmlelement/change_event/index.md:{{Specifications("api.GlobalEventHandlers.onchange")}}
./files/en-us/web/api/background_tasks_api/index.md:{{Specifications("api.Window.requestIdleCallback")}}
./files/en-us/web/api/web_share_api/index.md:{{Specifications("api.Navigator.share")}}
./files/en-us/web/api/remote_playback_api/index.md:{{Specifications("api.RemotePlayback")}}
./files/en-us/web/api/canvas_api/index.md:{{Specifications("html.elements.canvas")}}
./files/en-us/web/api/css_painting_api/index.md:{{Specifications("api.PaintWorkletGlobalScope")}}
./files/en-us/web/api/compression_streams_api/index.md:{{Specifications("api.CompressionStream")}}
./files/en-us/web/api/css_counter_styles/index.md:{{Specifications("api.CSSCounterStyleRule")}}
./files/en-us/web/api/geolocation_api/index.md:{{Specifications("api.Geolocation")}}
./files/en-us/web/api/badging_api/index.md:{{Specifications("api.Navigator.setAppBadge")}}
./files/en-us/web/api/notifications_api/using_the_notifications_api/index.md:{{Specifications("api.Notification")}}
./files/en-us/web/api/background_synchronization_api/index.md:{{Specifications("api.SyncManager")}}
./files/en-us/web/api/cookie_store_api/index.md:{{Specifications("api.CookieStore")}}
./files/en-us/web/api/serviceworkerglobalscope/sync_event/index.md:{{Specifications("api.SyncEvent")}}
./files/en-us/web/api/broadcast_channel_api/index.md:{{Specifications("api.BroadcastChannel")}}
./files/en-us/web/api/xmlhttprequest/using_xmlhttprequest/index.md:{{Specifications("api.XMLHttpRequest")}}
./files/en-us/web/api/xmlhttprequest/html_in_xmlhttprequest/index.md:{{Specifications("api.XMLHttpRequest")}}
./files/en-us/web/api/visual_viewport_api/index.md:{{Specifications("api.VisualViewport")}}
./files/en-us/web/api/beacon_api/index.md:{{Specifications("api.Navigator.sendBeacon")}}
./files/en-us/web/api/credential_management_api/index.md:{{Specifications("api.Credential")}}
./files/en-us/web/api/css/factory_functions/index.md:{{Specifications("api.CSS")}}
./files/en-us/web/api/channel_messaging_api/index.md:{{Specifications("api.MessageChannel")}}
./files/en-us/web/api/channel_messaging_api/using_channel_messaging/index.md:{{Specifications("api.MessageChannel")}}
./files/en-us/web/api/resize_observer_api/index.md:{{Specifications("api.ResizeObserver")}}
./files/en-us/web/api/contact_picker_api/index.md:{{Specifications("api.ContactsManager")}}
./files/en-us/web/api/clipboard_api/index.md:{{Specifications("api.Clipboard")}}
./files/en-us/web/api/constraint_validation/index.md:{{Specifications("api.ValidityState")}}
./files/en-us/web/api/content_index_api/index.md:{{Specifications("api.ContentIndex")}}
./files/en-us/web/http/headers/want-digest/index.md:{{Specifications("http.headers.Want-Digest")}}
./files/en-us/web/http/headers/digest/index.md:{{Specifications("http.headers.Digest")}}
./files/en-us/web/css/break-inside/index.md:{{Specifications("css.properties.break-inside.multicol_context")}}
./files/en-us/web/css/column-gap/index.md:{{Specifications("css.properties.column-gap.grid_context")}}
./files/en-us/web/css/break-before/index.md:{{Specifications("css.properties.break-before.multicol_context")}}
./files/en-us/web/css/align-items/index.md:{{Specifications("css.properties.align-items.grid_context")}}
./files/en-us/web/css/gap/index.md:{{Specifications("css.properties.gap.grid_context")}}
./files/en-us/web/css/row-gap/index.md:{{Specifications("css.properties.row-gap.grid_context")}}
./files/en-us/web/css/break-after/index.md:{{Specifications("css.properties.break-after.multicol_context")}}
./files/en-us/web/css/justify-self/index.md:{{Specifications("css.properties.justify-self.grid_context")}}
./files/en-us/web/css/align-self/index.md:{{Specifications("css.properties.align-self.grid_context")}}
./files/en-us/web/css/justify-items/index.md:{{Specifications("css.properties.justify-items.grid_context")}}
./files/en-us/web/css/align-content/index.md:{{Specifications("css.properties.align-content.grid_context")}}
./files/en-us/web/css/justify-content/index.md:{{Specifications("css.properties.justify-content.grid_context")}}
./files/en-us/mdn/structures/specification_tables/index.md:{{Specifications("css.properties.text-align")}}
./files/en-us/mdn/structures/specification_tables/index.md:you can pass the browser-compat query directly to the macro: `\{{Specifications("css.properties.text-align")}}`
./files/en-us/mdn/structures/page_types/api_landing_page_template/index.md:{{Specifications("path.to.feature.Interface_1")}}
./files/en-us/mdn/structures/page_types/api_landing_page_template/index.md:{{Specifications("path.to.feature.Interface_2")}}

There seem to be a few different cases here.

Most of these seem to be for pages that don't have a BCD entry, like the API overview pages. We should be able to replace these usages with spec-urls now we have it.

Some more are for the CSS properties like justify-self, that have subfeatures for "supported in flex"/"supported in grid", and browser-compat points to the top-level, like https://github.com/mdn/content/blob/main/files/en-us/web/css/justify-self/index.md?plain=1#L10 while the spec table wants the spec out of the subfeature, like https://github.com/mdn/content/blob/main/files/en-us/web/css/justify-self/index.md?plain=1#L192. What would happen here if we had behavior where if browser-compat had subfeatures that had spec URLs, Yari slurped all of the spec URLs out of the subfeatures and put them in the rendered spec table? We would also need to discard duplicates (it turns out that css.properties.justify-self.grid_context and css.properties.justify-self.flex_context actually point to the same spec). Would that work?

But there are a few usages that are tricky. For instance: https://developer.mozilla.org/en-US/docs/Web/Web_Components#specifications . Here it seems like we want to say:

This is not the normal case, where a page documents XYZ_feature and so we want the spec table, and BCD table, etc, for XYZ_feature. It's a special case where a page wants to include a spec table for an arbitrary feature, in the course of telling some other story.

I think we need to decide if we want to support this kind of thing or not. I would personally try to avoid it (Web Components is a weird page that is pretending to be an API overview page but isn't, really).

But if we do decide to allow this kind of arbitrary inclusion of spec tables, then it should trump anything else, and say: if {{Specifications}} has an argument, just use that and don't look at the front matter at all. Because it's a very explicit "override" in the prose content, like !important in CSS.

@hamishwillee
Copy link
Contributor

@wbamberg There is a lot of good points in ^^^ but I worry that @schalkneethling will miss them as this is a closed PR.

I like the idea of getting rid of {{Specifications("a key")}}, in particular in overview pages. But perhaps a better approach would be to instead allow multiple BCD keys to be specified in the same way as we are suggesting multiple spec-urls. BCD data is always better to use.

I very much like both spec-url and bcd key to be reported as a flaw.

@sideshowbarker
Copy link
Member Author

But perhaps a better approach would be to instead allow multiple BCD keys to be specified in the same way as we are suggesting multiple spec-urls. BCD data is always better to use.

Yeah, I have been thinking the same thing.

So I think we should update our BCD processing code to handle browser-compat values that contain a YAML array — but I don’t think we should block these spec-urls PRs on getting that change made first. We can merge those PRs and then later do the multi-value browser-compat change and either go back and drop the spec-urls keys from the docs that no longer need them — though if we make the logic be that the spec-urls is ignored if there’s a browser-compat key, then we wouldn’t necessarily even need to drop the spec-urls keys (they wouldn’t break anything but would just be cruft — though I agree it’d still be better to remove them).

@sideshowbarker
Copy link
Member Author

About making it an error for a document to have spec-urls key when it already has a browser-compat key: I’ve run into some cases where it seems like it should not be an error.

The general case is when there’s a browser-compat key but the specified value doesn’t actually exist yet in BCD. Arguably I guess we should be fixing those by getting BCD updated. But in the meantime, if we disallow spec-urls in those cases, then along with have no Browser Compatibility table, we’d also have no Specifications section.

So, allowing spec-urls in those cases means that at least we can still have a Specifications sections in those documents.

@wbamberg
Copy link
Collaborator

My main concern here is simplicity for authors. This is looking like a pretty complicated system right now with lots of different ways to provide a spec URL. It's fine to have some extra complexity as a transitional thing, if we have a desired final state that we like, and a plan for getting there.

So I think we need (1) a description of where we want to end up with spec URLs, (2) a description of where we are now, and (3) a plan for how to get from here to there. I'm happy to try to start this off if you like.

@schalkneethling
Copy link

Hey @wbamberg and @sideshowbarker. I have been following along here as much as I can, feel free to move this to either a discussion or new issue and link it here. Thanks!

@sideshowbarker
Copy link
Member Author

So I think we need (1) a description of where we want to end up with spec URLs, (2) a description of where we are now, and (3) a plan for how to get from here to there. I'm happy to try to start this off if you like.

Yes, please, if you could start it off, that’d be great, and much appreciated.

@ddbeck
Copy link
Contributor

ddbeck commented Mar 1, 2022

I'm stealing this a little. I went ahead and wrote up a possible vision for this stuff: #5347. That might cover (1), but not (2) or (3), but I think (3) is the last piece anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves an existing feature.
Projects
Development

Successfully merging this pull request may close these issues.

Allow for providing spec URLs when there is no BCD table
7 participants