Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

refactor(macros/CSS): Use mdn‑data as a node package #1126

Merged
merged 4 commits into from
Jun 6, 2019

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Apr 18, 2019

As requested by @jwhitlock in #1043 (comment).

This will prevent changes to or outages of the GitHub API from causing CSS macros to fail.

review?(@davidflanagan, @wbamberg)


Note that this will require mdn‑data to switch to a weekly release model like the one used by BCD.

@ExE-Boss ExE-Boss changed the title refactor: Use mdn‑data as a node package refactor(macros/CSS): Use mdn‑data as a node package Apr 18, 2019
package.json Show resolved Hide resolved
Copy link
Contributor

@davidflanagan davidflanagan left a comment

Choose a reason for hiding this comment

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

I did not realize that we were hitting githubusercontent.com like this each time we rendered one of these macros. This is a much better way to do it. Thank you for taking it on @ExE-Boss .

I had one nit about not using require in the middle of a file, and a more general request that you not use this PR as a place to go changing the capitalization of filenames.

I'm concerned about the size of the changes to npm-shrinkwrap.json, but I suspect that that is just because it was out of date and needed to be updated.

Since I don't know anything about the mdn-data repo, I'm going to leave final approval of the patch to @wbamberg

macros/CSSSyntax.ejs Outdated Show resolved Hide resolved
tests/macros/CSSxRef.test.js Outdated Show resolved Hide resolved
npm-shrinkwrap.json Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss force-pushed the refactor/use-mdn-data branch from 1e4902e to 0e11646 Compare April 19, 2019 16:46
@wbamberg wbamberg self-requested a review April 28, 2019 22:52
Copy link

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Yes, this is a thing we ought to do, with a couple of reservations.

The main reasons I've not done it before are:

  1. we don't have tests for any of these macros, I'm reluctant to update them without tests, and some of them are very complex to the point of being more or less untestable.

  2. I think the value of these macros is questionable. See for example https://discourse.mozilla.org/t/updates-to-the-css-info-boxes/30855, and it has been agreed at times that we should stop showing formal syntax on pages. I'd love us to sit down and think from first principles about what we want in the CSS docs and build macros (and backing data stores) that support just that.

  3. relatedly, it's not obvious to me that mdn/data is currently very useful for us. We don't successfully maintain it (even to the extent of making regular releases, which as you point out would become a necessity after this change) and updating the formal syntax is a maintenance burden which (see above) isn't obviously worth it. (I do understand that other parties use mdn/data, and that ought to be factored in, but that fact doesn't make it untouchable.) It's notable that BCD has been successful, and mdn/data hasn't, and I would say that's partly because BCD has a clear owner and a clear mission, while mdn/data has neither.

If we merge this, who will commit to releasing the package weekly? Apart from that though... 2 and 3 don't have much bearing on the PR either way I suppose, and as for 1, we could just ship this and see if it breaks :).

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Apr 30, 2019

Well, the exports of mdn-data/css match the data object structure: https://github.com/mdn/data/blob/master/css/index.js, so merging it should be fine.

Copy link

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Given #1126 (review), and following discussions in the content team, I'm happy with this from the content policy point of view. I'd still like an r+ from the dev team from the technical side though.

@escattone
Copy link
Contributor

@wbamberg I'll take a look.

Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

I like this change, but as @wbamberg commented earlier, the lack of tests for the changed macros is dangerous, and in fact, was the reason that the CSSInfo macro wasn't detected as broken, which it is.

macros/CSSInfo.ejs Show resolved Hide resolved
@escattone
Copy link
Contributor

FYI, forgot to add that the other macros seemed to work fine in my local dev environment.

Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

Verified, within my local dev environment, that the cssinfo macro works now for both English and a translation (French).

This is a nice step forward, thank you @ExE-Boss, @wbamberg, @davidflanagan, and @peterbe!

@escattone escattone merged commit fd3d002 into mdn:master Jun 6, 2019
@wbamberg
Copy link

wbamberg commented Jun 6, 2019

Indeed, this is a good thing to have merged. Thanks everyone for working on it!

@ExE-Boss ExE-Boss deleted the refactor/use-mdn-data branch June 6, 2019 21:20
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jun 7, 2019

Note that if ::part(…), @supports or any other feature that got updated since mdn‑data@v2.0.2 was released has its page re‑rendered, the updated data will be lost until the next mdn‑data version gets released to npm and deployed to production.

@wbamberg
Copy link

wbamberg commented Jun 7, 2019

Good point. I just released mdn-data 2.0.3, which ought to help a bit with that.

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.

5 participants