Change {{compat}} to work with new BCD data structure #272

Elchi3 · 2017-08-02T17:59:47Z

For more details see mdn/browser-compat-data#304
and mdn/browser-compat-data#283

To test this you'll have to:

Follow the steps to generate the new data structure in Improved data structure (issue 283) browser-compat-data#304
Copy the files with the new structure to kuma/kumascript/node_modules/mdn-browser-compat-data/
Call the macros on pages like you did before, optionally with the new second depth parameter.

This can't land at the moment. Maybe not yet exhaustive, but a start of a checklist for this:

Patch WebExtension compat macro (likely to be added to this PR).
Get this PR reviewed / approved.
Land new data structure at the BCD repo (WIP see the BCD 304 PR).
Publish new npm package containing new structure.
Merge this PR.
Point Kuma to latest KumaScript master
Deploy.
Win.

For more details see mdn/browser-compat-data#304 and mdn/browser-compat-data#283

wbamberg · 2017-08-10T04:31:33Z

To test this you'll have to:

This isn't working for me. I've generated new-style data using npm run convert http, and the data looks all right. I've created "node_modules/mdn-browser-compat-data/" under my local kuma installation (the "node_modules" directory didn't exist, is that expected?) and copied "http" under it. So I have this:

kuma/kumascript/node_modules/mdn-browser-compat-data/http/methods.json

I point my local KS at your clone/branch, and check that the macros are the same version as in this PR.

Then I try this in a page:

{{compat("http.methods.CONNECT")}}

This gives me this output:

No compatibility data found. Please contribute data for "http.methods.CONNECT" (depth: 1) to the MDN compatibility data repository.

Trying to debug a bit, it looks like the bcd object I am getting is still the old one:

Object.keys(bcd)[0]; // -> "version"

Elchi3 · 2017-08-10T10:49:49Z

If you change node_module files you'll have to restart KumaScript (or Docker) in order to pick up the changed module files in KS.

cc/ @escattone who might have additional hints on how to fiddle with modules locally.

escattone · 2017-08-10T17:18:43Z

Sorry @wbamberg and @Elchi3 , looking into this more, I realized that I was confused about how NODE_PATH was used by node when loading modules. When the kumascript container is run (using docker-compose up -d), it normally uses the node_modules folder under it's root directory. I didn't realize that if you had a node_modules folder under the kumascript directory (which is mounted at /app in the kumascript container), it will use that instead. Sorry for my comments in IRC that may have added to the confusion.

I think @Elchi3 is absolutely right-on here, in that @wbamberg I think all you should have to do is restart the kumascript container using docker-compose restart kumascript.

wbamberg · 2017-08-11T17:32:42Z

I think @Elchi3 is absolutely right-on here, in that @wbamberg I think all you should have to do is restart the kumascript container using docker-compose restart kumascript.

Actually, I can't get this to work. I've resorted to pasting thousands of lines of JSON into Compat.ejs.

wbamberg · 2017-08-11T21:25:59Z

I've not yet reviewed the code here, just looking at the output. I've generated a few representative tables from the WebExtension data:

All manifest keys, depth of 1, no notes.
All manifest keys, depth of 1, with notes.
All manifest keys, depth of 2, with notes.
manifest.chrome_settings_overrides, depth of 1, with notes.

)

The first thing is that I'm not sure whether the font colors for the "standard" table combine well with the background colors for WE tables. Especially red font on reddish background for "No".

The second thing is that this doesn't exactly replace the "aggregate tables". I'm not sure that the approach this takes, where there's just one type of table, and you use depth and note-hiding to adjust the output, is viable. I think of aggregate tables and feature tables as quite distinct things.

The closest thing to a table like this one: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json#Browser_compatibility is I think (2) in the screenshot - "All manifest keys, depth of 1, with notes". But there are a couple of differences:

the aggregate tables link the name of each feature to its page. I think this is useful, especially if the aggregate table might appear somewhere other than the main API page.
the aggregate tables don't include all notes. They used to, actually, but it was quite unreadable in lots of cases. Instead, they say a feature is supported if any of it is supported, then add a linked asterisk if there are notes or if there are subfeatures (which typically indicate partial support). Tables like (2) don't indicate partial support at all in cases where partial support is represented using subfeatures rather than with notes.

Feature tables are supposed to provide complete info for a given "feature", where a feature is basically a thing that has its own docs page. So I guess feature tables should iterate all the way through subfeatures, no matter how deep? Should there be a way to indicate that in the macro call? Or just pass Number.MAX_SAFE_INTEGER?

Then we have to deal with flattening though.

The flattening implemented here loses information that makes the tables be not understandable. For example, in (3), "homepage" is a subfeature of "chrome_settings_overrides" but that's not indicated. What if another feature also has a subfeature called "homepage" - which one is which?

We talked about this I think, and the most obvious approach seems to be: if the subfeature has a description, then write something like:
"feature: subfeature description". For example:

Promise: constructor requires the new operator

If the subfeature does not have a description, then write something like "feature.subfeature". For example:

chrome_settings_overrides.homepage

or:

chrome_settings_overrides.search_provider.alternate_urls

The last thing I can see here is that given a macro call like:

compat("webextensions.manifest.chrome_settings_overrides", 1)

...if chrome_settings_overrides has its own __compat (as it does in this case) then that should be shown as Basic support, not as chrome_settings_overrides. I think it makes more sense.

wbamberg

I think this is mostly correct, although I think the interaction of CSS classes and complex support statements might not work.

But as I said in the last comment, I'm not sure if this is quite the output that we want.

wbamberg · 2017-08-11T22:41:45Z

macros/Compat.ejs

-  $0 – A query string indicating for which feature to retrieve compat data for.
+$0 – A query string indicating for which feature to retrieve compat data for.
+$1 – A depth setting indicating how deep sub features should be added to the table
+(flattened, default: 1 level)


You could perhaps clarify this to say that (IIRC) 1 gives you compat data for the given feature, if it has any, plus compat data for any of its immediate children.

wbamberg · 2017-08-11T22:43:07Z

macros/Compat.ejs

+$0 – A query string indicating for which feature to retrieve compat data for.
+$1 – A depth setting indicating how deep sub features should be added to the table
+(flattened, default: 1 level)
+$2 – A boolean setting whether to hide notes or not (defaults to false).


I think a double negative (hideNotes==false, so show notes by default) is harder to understand than a single positive (showNotes defaulting to true).

wbamberg · 2017-08-11T22:45:55Z

macros/Compat.ejs

-`browserPlatformType` is either "mobile" or "desktop"
-`browserNames` is either the "mobileBrowsers" or "desktopBrowsers" object
+`browserPlatformType` is either "mobile", "desktop" or "webextensions"
+`browserNames` is either the "mobileBrowsers", "desktopBrowsers" or "webExtBrowserNames" object


I think it's odd that this takes both these arguments, when you could derive one from the other. Why not just pass browserPlatformType and use that to look up the right browserNames in a map-type thing?

This is implicit at line 96 where you actually use the webExtBrowserNames variable directly, instead of the browserNames argument

I refactored this to have a browser map and to only pass browserPlatformType to the various functions.

wbamberg · 2017-08-11T22:54:25Z

macros/Compat.ejs

-      return `<span title="${localize(compatStrings, 'supportsShort_unknown_title')}"
-              style="color: rgb(255, 153, 0);">${localize(compatStrings, 'supportsShort_unknown')}</span>`;
-      break;
+      return {class: 'unknown-support', text: `<span title="${localize(compatStrings, 'supportsShort_unknown_title')}"


This whole business of changing this from returning a string to returning an object is a bit awkward IMO.

First the comment at line 112 talks about a string, so does the function name. Then it breaks the convention in this file generally, where writeXYZ() functions return a string called output.

Then I think it also breaks when you have multiple support statements, or when you have version_added and version_removed.

For example, IIUC this is a valid support_statement structure:

{ "version_added": "3.5", "version_removed": "9" }

This says that the feature is not currently supported, but as I understand the logic in here, the call at line 211 will get us a CSS class of full-support.

I think it might be cleaner to have separate functions to get the text string and to get the CSS class. Then you could call the CSS class one from writeSupportCells, which AIUI is the only place you need to know the class.

Also, is it a problem that class is a JS keyword? Anyway, that doesn't seem like very good practice.

Yes, I originally backported this from the webext compat macro, but you are right: The whole business of getting a CSS class is actually not straight forward and is now in an own function getSupportClass.

wbamberg · 2017-08-11T22:56:28Z

macros/Compat.ejs

    } else { // this browser has no info, it's unknown
-      supportInfo = writeSupportInfo(null);
+    supportInfo = writeSupportInfo(null);


indentation

wbamberg · 2017-08-11T22:58:13Z

macros/Compat.ejs

-
-  for (let subfeature of Object.keys(json_subfeature_set)) {
-    let support = json_subfeature_set[subfeature]['support'];
+  for (let row of features) {


I think it's a bit weird to refer to this features global here, especially where it's not initialized above this function. I guess this is just a style thing, but I would have had it as a parameter.

wbamberg · 2017-08-11T23:02:49Z

macros/Compat.ejs

+function traverseFeatures(obj, depth, identifier) {
+  depth--;
+  if (depth >= 0) {
+    for (i in obj) {


for (let i in obj)

wbamberg · 2017-08-11T23:04:41Z

macros/Compat.ejs

+    for (i in obj) {
+      if (!!obj[i] && typeof(obj[i])=="object" && i !== '__compat') {
+        if (obj[i].__compat) {
+          // [identifier + '.' + i]


I don't understand this comment, and I'm not sure what identifier is doing here in general. Should [i] in the next line be [identifier], so you get dotted identifiers for child features?

wbamberg · 2017-08-11T23:07:09Z

macros/WebExtAllCompatTables.ejs


 function writeTableForModule(data, moduleName) {
-  var table = `<h2>${moduleName}</h2>`;
-  table += template("WebExtBrowserCompatTable", [data, `${webExtAPIBaseUrl}/${moduleName}`]);
+  var table = `<h2><a href="${webExtAPIBaseUrl}/${moduleName}">${moduleName}</a></h2>`;


I have heard, that it's not considered to be good practice in MDN to have headers be links. I think I tend to agree with this.

I agree. I will remove this when I have a implemented a way to have feature labels linked to MDN pages in "aggregate" tables.

- remove text coloring (will be redone in the redesign anyway) - Update depth parameter comment - change hideNotes to showNotes - Create a map of browsers - Change writeTableHead, writeTable and writeSupportCells to use a map of browsers and browserPlatformType - Change getVersionString back to just return a string - Add a function getSupportClass that returns a proper CSS class for a cell - Implement labels that represent the hierarchies that are lost due to flattening for the 2D table

Elchi3 · 2017-08-15T12:24:22Z

Thanks a lot for your extensive feedback! I've added a commit that does some cleanup regarding the code style and the nits you've mentioned.

Regarding the general output of this, I think I see two things that we want to implement before this might be good to go.

Have an "aggregate mode" (instead of the showNotes param). When this is true, do the following:
- Link features to their MDN pages (likely through a mapping). This means that we'll want aggregate tables to always only display features that have MDN pages. (I'm really unsure how we would otherwise decide when to attempt to find an MDN page for a feature and when not)
- Show an asterisk if there is partial support or notes in the feature or in its sub feature
- Don't show notes
Show "Basic support" for calls like compat("webextensions.manifest.chrome_settings_overrides", 1 and features that have __compat. I'm not yet 100% sure when we want to display the feature name and not basic support. I will play with this a bit. Maybe aggregateMode should always display the feature name instead of "Basic support".

Another thing that I read in your feedback but I don't think it's critical at the moment:

Add a way to iterate through all subfeatures (could currently be done using a depth param of 100 or so)

wbamberg · 2017-08-15T15:00:22Z

Add a way to iterate through all subfeatures (could currently be done using a depth param of 100 or so)

What do you think of: if the depth param is omitted, iterate through all subfeatures? This saves us from some hacky approach, and I think this will probably be the most common choice (for feature tables anyway).

Elchi3 · 2017-08-16T11:21:23Z

Link features to their MDN pages (likely through a mapping). This means that we'll want aggregate tables to always only display features that have MDN pages. (I'm really unsure how we would otherwise decide when to attempt to find an MDN page for a feature and when not)

I think this would be cleaner: mdn/browser-compat-data#320.

* re-introduce "Basic support" * introduce aggregateMode param (instead of showNotes) * remove links from WebExt modules headlines

Elchi3 · 2017-08-17T12:16:24Z

I re-introduced an aggregateMode and "Basic support".
An asterisks is shown now when there are notes.
To link features, this expects mdn_url in the data, which I plan to add over in the BCD repo soon.

I think this gives us mostly what we have right now. Given that we want to redesign the tables soon and that this contains already many major changes, I think we could live with minor glitches. Can you give this another look?

wbamberg · 2017-08-17T16:49:29Z

For comparison, here's the output with the same macro calls as in #272 (comment):

wbamberg · 2017-08-17T17:22:22Z

Some comments on the output:

the biggest issue is that in the old macro the asterisk is shown when a feature has notes or when it has any subfeatures, since this usually indicate partial support. I think this is not implemented here (you can see, for example, that chrome_settings_overrides does not get an asterisk for Firefox in the screenshot above, but does get one in https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json?document_saved=true#Browser_compatibility.
in the old tables, the asterisk is linked to the page#compat-table. I guess this doesn't matter so much if the feature's name is linked to the page.

Apart from that, it looks great! I guess as part of updating the data we'll be adding mdn_url?

For the Extensions docs, will be update every page to call Compat()? As I said before I think, I don't mind us doing this, but only want to do it once. So if we think the table redesign has a significant chance of changing the macro's interface, I'd prefer to shim the old macros in this revision so we don't have to edit any pages.

wbamberg · 2017-08-17T17:30:48Z

macros/Compat.ejs

+  let cssClass = 'unknown-support';
+
+  if (Array.isArray(supportInfo)) {
+    // the first entry should be the most relevant/recent and will be treated as "the truth"


Is it the first, or the last?

I think this should be the first. When I look at the table cells, I think I like seeing the most relevant compat data first. So, for example, if there is unpreffed support from Firefox 51, I think that is the most relevant information. Then only after that I'll see that there was also preffed support from versions 34-50.

I'm thinking of something like this, in which as far as I can tell, the feature is no longer supported. But the syntax has multiple support statements, and only looking at the first would make it look like it's supported.

wbamberg

This looks good, but:

this question: Change {{compat}} to work with new BCD data structure #272 (review) should be answered (I think the code is wrong, but I might be the one that's wrong :)
I'd like the asterisks to account for subfeatures, as noted here: Change {{compat}} to work with new BCD data structure #272 (comment).
I don't think asterisk-linking is needed to land this.
I'd like to consider explicitly whether to shim the WE macros now, in the expectation that we will have to change the compat macro interfaces when table redesign happens.

Elchi3 · 2017-08-18T09:46:22Z

I'd like the asterisks to account for subfeatures, as noted here: #272 (comment).

Yes, that makes sense to me. I'll try to get this implemented.

I don't think asterisk-linking is needed to land this.

Yes, I think partial support might be indicated differently in the redesigned tables (I think I remember a yellow cell background instead of an asterisk), so lets not worry about this one.

I'd like to consider explicitly whether to shim the WE macros now, in the expectation that we will have to change the compat macro interfaces when table redesign happens.

I think that the {{Compat}} macro parameter aren't changing as they are about data querying and not so much about data displaying. They are also used this way on many web docs already. Maybe we will fiddle again with the aggregateMode (which in fact affects the displaying), but that isn't used on the majority of the pages I think. I'm happy to update the Extension pages for this change.

Is a shim straight-forward to implement? I would love to get rid of the whole URL hackery that is currently done to determine what to query and I expect most {{compat}} calls to just have the queryString parameter, which we always want I think. So I expect most calls using just one parameter, like {{compat("webextensions.api.alarms.Alarm")}} and I think we want this in any situation going forward.

wbamberg · 2017-08-18T15:53:47Z

Is a shim straight-forward to implement?

I think so (without having actually tried :). The WebExt macros would just map the current URL onto the query string and call Compat.ejs.

I would love to get rid of the whole URL hackery that is currently done to determine what to query

Agreed, but this will involve editing 400-odd pages, and I don't want to do that more than once. So unless we are confident that the table redesign will not change the macro interface, I would lean towards shimming it until the table redesign.

and I expect most {{compat}} calls to just have the queryString parameter, which we always want I think. So I expect most calls using just one parameter, like {{compat("webextensions.api.alarms.Alarm")}} and I think we want this in any situation going forward.

Yes, I guess so.

Elchi3 · 2017-08-21T09:55:44Z

I've added a shim for the WebExt macro and implemented a partial_support flag that is added if support of a subfeature has a note or isn't of the same compat as the main feature. PTAL :)

wbamberg

The shim will never generate an aggregate table. The old macro would generate an aggregate table table when the data didn't contain "__compat": https://github.com/mdn/kumascript/blob/master/macros/WebExtBrowserCompatTable.ejs#L244.

Since we're now representing that explicitly, the shim needs to figure out whether we want an aggregate table or a feature table. I think a reasonable heuristic is that if the page is directly under "API", build an aggregate table.

Apart from that, I think this is good!

wbamberg · 2017-08-22T18:48:16Z

macros/Compat.ejs

+              }
+            }
+          }
+


Looks good!

Elchi3 · 2017-08-23T11:33:36Z

Good catch, I completely forgot about the aggregate tables for the main Extension API pages. Should be fixed now.

wbamberg

LGTM, thanks for your patience @Elchi3 !

* mdn/kumascript#272 - Compat, etc. - new BCD data structure * mdn/kumascript#284 - SpecName, spec2: Remove outdated specs * mdn/kumascript#287 - ListGroups: new macro * mdn/kumascript#288 - HTTPSidebar: Add HTTP Guide, Protocol upgrade * mdn/kumascript#289 - CSS_Ref: Add shape and basic-shape cases

Change {{compat}} to work with new data structure

41976ce

For more details see mdn/browser-compat-data#304 and mdn/browser-compat-data#283

Elchi3 requested a review from wbamberg August 2, 2017 17:59

Make WebExtensions use {{compat}} macro

b94a81c

wbamberg suggested changes Aug 11, 2017

View reviewed changes

Address more review comments

85bf0da

* re-introduce "Basic support" * introduce aggregateMode param (instead of showNotes) * remove links from WebExt modules headlines

wbamberg reviewed Aug 17, 2017

View reviewed changes

wbamberg suggested changes Aug 17, 2017

View reviewed changes

Elchi3 added 2 commits August 21, 2017 10:14

Add a shim for WebExtBrowserCompat

07ca33c

Indicate partial support by using a flag on main features

92b81e6

wbamberg suggested changes Aug 22, 2017

View reviewed changes

macros/Compat.ejs

}

}

}

Copy link

wbamberg Aug 22, 2017

Choose a reason for hiding this comment

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

Looks good!

Use aggregate tables for main Extension API pages

fb63a42

wbamberg approved these changes Aug 23, 2017

View reviewed changes

Elchi3 changed the title ~~[DO NOT MERGE] Change {{compat}} to work with new data structure~~ Change {{compat}} to work with new BCD data structure Aug 24, 2017

Elchi3 merged commit e753864 into mdn:master Aug 24, 2017

jwhitlock mentioned this pull request Aug 24, 2017

Update locales, kumascript PR 272, 284, 287-289 mdn/kuma#4382

Merged

Elchi3 deleted the new-bcd-structure branch August 24, 2017 15:44

Change {{compat}} to work with new BCD data structure #272

Change {{compat}} to work with new BCD data structure #272

Conversation

Elchi3 commented Aug 2, 2017 • edited Loading

wbamberg commented Aug 10, 2017

Elchi3 commented Aug 10, 2017

escattone commented Aug 10, 2017 • edited Loading

wbamberg commented Aug 11, 2017 • edited Loading

wbamberg commented Aug 11, 2017

wbamberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elchi3 commented Aug 15, 2017 • edited Loading

wbamberg commented Aug 15, 2017

Elchi3 commented Aug 16, 2017

Elchi3 commented Aug 17, 2017

wbamberg commented Aug 17, 2017

wbamberg commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbamberg left a comment • edited Loading

Choose a reason for hiding this comment

Elchi3 commented Aug 18, 2017

wbamberg commented Aug 18, 2017

Elchi3 commented Aug 21, 2017

wbamberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elchi3 commented Aug 23, 2017

wbamberg left a comment

Choose a reason for hiding this comment

Elchi3 commented Aug 2, 2017 •

edited

Loading

escattone commented Aug 10, 2017 •

edited

Loading

wbamberg commented Aug 11, 2017 •

edited

Loading

Elchi3 commented Aug 15, 2017 •

edited

Loading

wbamberg left a comment •

edited

Loading