Skip to content
This repository has been archived by the owner on Sep 7, 2021. It is now read-only.

Support 'generated.status' in build-json #354

Closed
wants to merge 7 commits into from

Conversation

wbamberg
Copy link

@wbamberg wbamberg commented Mar 11, 2020

This PR addresses the content side of supporting banners/headers, as tracked by #306.

It adds support for a new status ingredient. Its value is derived from the specifications and browser-compatibility properties, so is made a new sort of ingredient, generated: which means it appears in recipes but not in front matter: writers explicitly don't have to worry about it.

This is from @ddbeck 's suggestion here: #306 (comment).

It's a bit inefficient because it queries the BCD twice for the data. But I think this is pretty quick and it didn't really seem worth caching it, just for this. But maybe it is? But anyway I don't want to cache all the BCD query lookups, as we do for related_content, because unlike related_content, BCD JSONs are not typically shared between different pages.

It'll add a new JSON object like:

      "type": "status",
      "value": {
        "non_standard": true,
        "experimental": false,
        "deprecated": true
      }
    }

...placed at the start of the body:

  "body": [
    {
      "type": "status",
      "value": {
        "non_standard": true,
        "experimental": false,
        "deprecated": true
      }
    },
    {
      "type": "prose",
      "value": {
        "title": "Short description",
        "id": "short_description",
        "content": "<p>...</p>"
      }
    },

@wbamberg wbamberg requested review from ddbeck and peterbe March 11, 2020 00:17
Copy link
Contributor

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

This looks good to me. I've left one line comment concerning a finer point of the BCD schema, but it's not a big thing. Thank you, Will!

@@ -0,0 +1,12 @@
function packageStatus(specifications, bcd) {
const bcdHasStatus = !!bcd.__compat && !!bcd.__compat.status;
Copy link
Contributor

Choose a reason for hiding this comment

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

!! is a bit cryptic. I'd prefer something more explicit here (bcd.__compat !== undefined or whatever we're getting at).

Also, the BCD schema requires __compat to have a status object. Seems like we should be able to trust BCD to not break Stumptown.

Copy link
Author

Choose a reason for hiding this comment

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

Also, the BCD schema requires __compat to have a status object. Seems like we should be able to trust BCD to not break Stumptown.

Oh, then the docs are wrong: https://github.com/mdn/browser-compat-data/blob/master/schemas/compat-data-schema.md#the-__compat-object and https://github.com/mdn/browser-compat-data/blob/master/schemas/compat-data-schema.md#status-information

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @ddbeck ! I'm glad BCD mandates status, and have correspondingly tried to simplify this bit of the code.

Copy link
Author

Choose a reason for hiding this comment

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

Of course this really wants optional chaining (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining) but that's not in Node yet apparently.

scripts/build-json/resolve-status.js Outdated Show resolved Hide resolved
wbamberg and others added 2 commits March 12, 2020 10:10
Co-Authored-By: Daniel D. Beck <daniel@ddbeck.com>
@wbamberg
Copy link
Author

wbamberg commented Mar 12, 2020

So we can visually check the rendered banners, I added a fake JS class "TestBanners", which borrows the compat data from Intl.ListFormat, so it gets tagged "experimental". I've given it a constructor TestBanners() which also uses that compat data, but additionally is marked "non-standard", so we can see what multiple banners are like.

I've had to give them the Array URL, because one of the CI checks requires that mdn_url is a real MDN page.

I've also filed mdn/yari#380 for the renderer side, in case you want to see what it looks like.

@wbamberg
Copy link
Author

@ddbeck , in mdn/yari#380, @peterbe asked for us to represent status as an array of constants rather than an object. This seems like a good idea, so 24c96b7 tries to do that.

It also omits status if it's empty: in most cases it will be empty, and it does not seem worthwhile shipping an empty array around, and it makes things more complicated for the renderer.

I know it seems/is pointless, but I'd appreciate a review if you have time, so we can close this issue. Thanks!

Copy link
Contributor

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Yeah, that's fine. I follow why it makes a difference (something about waste? I didn't get it), but I can still follow the semantics of an array on this side, so it works. 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants