Create amp-validator-rules package #377

fstanis · 2019-07-05T16:55:20Z

This creates a new tool, amp-validator-query, that makes it easier to query AMP validator JSON rules to extract information.

ithinkihaveacat

Cool, dropped some comments! I think either @sebastianbenz or @andreban should review as well. (I also don't have merge rights.)

packages/validator-query/README.md

+#### ES Module (Browser)
+
+```javascript
+import { load } from 'amp-toolbox-validator-query';


packages/validator-query/README.md

+#### CommonJs (Node)
+
+```javascript
+const { load } = require('amp-toolbox-validator-query');


ithinkihaveacat · 2019-07-08T10:53:56Z

packages/validator-query/README.md

+   * `noCache`: true to always fetch latest rules (by default, subsequent calls to `load` reuse the same result).
+   * `rules`: object to use locally specified rules instead of fetching them from the AMP CDN.
+   * `url`: override the URL where validator rules are fetched from.
+   * `source`: one of `'local'` (load rules from local file named "validator.json"), `'remote'` (fetch rules from CDN) or `'auto'` which is the default (tries looking for the local file first, then tries to fetch from CDN).


Not for this PR, but we should create some standard way to handle these options (noCache is also related).

Other places where a similar mechanism exists:

the linter, which essentially defaults to auto

runtime-version, which uses onebehindfetch

the validator (defaults to network)

… probably others

Migrated this to its own issue: #378

FWIW: this is the only toolbox package that works with validator.json (as opposed to validator.js).

In theory, this is meant to be a low-level interface to that file, so no other tool should ever need to load validator.json directly.

+1 to Michael's comment

packages/validator-query/package.json

@@ -0,0 +1,27 @@
+{
+  "name": "@ampproject/validator-query",


packages/validator-query/index.js

+  return cached;
+}
+
+module.exports = { load };


fstanis · 2019-07-08T12:15:24Z

packages/validator-query/spec/AmpValidatorQuerySpec.js

@@ -0,0 +1,171 @@
+/**


Well, it's not obvious from the log, but if I try to make a test break (e.g. change one expected result), npm test complains, so I'm quite confident it runs.

The pattern also matches this file if I'm reading it correctly (** should also work for .)?

You're right, comment withdrawn!

packages/validator-query/lib/rules/node.js

+ */
+
+const https = require('https');
+const {promisify} = require('util');


sebastianbenz

Awesome! Left a few comments inline.

Two higher level comments:

What are typical use cases for API? Are these covered by the two methods we provide? Would it make sense to always fetch rules per format? e.g.: ValidationRules.fetch('AMPHTML')?
Suggestion: can we rename the project to toolbox-validation-rules? I think this better explains what it does.

sebastianbenz · 2019-07-08T13:48:29Z

packages/validator-query/README.md

+#### ES Module (Browser)
+
+```javascript
+import { load } from '@ampproject/toolbox-validator-query';


Can we use a more descriptive name? Suggestion:

import { ValidationRules } from '@ampproject/toolbox-validator-query'; const validationRules = await ValidationRules.fetch()

Hm, I'm OK with renaming, but I have two problems with that:

fetch sounds like it misrepresents what that method does since it may not fetch anything, depending on the options provided.

What other methods would ValidationRules have? I'm kind of reluctant to expose the AmpValidatorQuery class itself since the end user shouldn't need to instantiate it on their own and I don't see any other functions we'd need. That said, I'm OK with having ValidationRules an object with a single method if you feel that makes sense.

The reason I prefer to export a class is that it becomes easier to test:

Users will use:

const validationRules = await ValidationRules.fetch()

For testing you can do:

const validationRules = new ValidationRules(fetchMock).fetch()

fetch was just a suggestion. What I like about it is that it makes it clear that this might perform a network request (as opposed to ValidationRules.get().

So, I don't see a reason to make the class public since testing is already possible by directly including (see AmpValidatorRulesSpec.js) - so this is the closest we can get to package-private constructor. The user can also specify a mock to the load function via the rules parameter (e.g. load({ rules: { ... } })). Exposing the class makes me feel like it can cause a lot of confusion.

Even better if we don't need to export a class, however, we still should export an object in that case:

module.exports = { fetch: ... (or whatever name we decide on) }

This is to keep the possibility to add more methods in the future and to be consistent with the other modules, e.g.:

const AmpOptimizer = require('@ampproject/toolbox-optimizer'); const ampOptimizer = AmpOptimizer.create();

Done, renamed to fetch and ensured it's exported as an object (it was already, but I made it clear in the readme).

sebastianbenz · 2019-07-08T13:49:12Z

packages/validator-query/README.md

+   * `noCache`: true to always fetch latest rules (by default, subsequent calls to `load` reuse the same result).
+   * `rules`: object to use locally specified rules instead of fetching them from the AMP CDN.
+   * `url`: override the URL where validator rules are fetched from.
+   * `source`: one of `'local'` (load rules from local file named "validator.json"), `'remote'` (fetch rules from CDN) or `'auto'` which is the default (tries looking for the local file first, then tries to fetch from CDN).


+1 to Michael's comment

sebastianbenz · 2019-07-08T13:53:53Z

packages/validator-query/lib/rules/node.js

+  return JSON.parse(data);
+}
+
+async function loadLocal() {


We should move this functionality into the core package (we need it for optimizer as well) and find a common approach. As a start, I'd suggest only using OneBehindFetch and add caching later.

Fixed, sort of. I consider the current loadRules.js temporary, so when we figure out what to do with #378, most of it will probably go away.

fstanis · 2019-07-08T14:43:00Z

packages/validator-query/spec/AmpValidatorQuerySpec.js

+});
+
+function makeQuery(rules) {
+  return new AmpValidatorQuery(


The unit test doesn't do any network requests - in fact, the AmpValidatorQuery can't fetch the rules, it expects them to be provided (load fetches them and passes them to AmpValidatorQuery). Here the rules are provided as a local object.

You're right, I was one step ahead in my mind.

fstanis · 2019-07-08T16:20:07Z

packages/validator-query/README.md

@@ -0,0 +1,57 @@
+# AMP-Toolbox Validator Query
+
+Makes it easier to query published AMP Validator rules and extract information


fstanis · 2019-07-09T17:16:12Z

packages/validator-query/README.md

+  const query = load();
+
+  // Get all tag names used in AMP for Email
+  const names = query.getTagsForFormat('AMP4EMAIL').map(tag => tag.tagName);


Linked to relevant proto definitions and added examples for all methods.

sebastianbenz · 2019-07-08T14:05:04Z

packages/validator-query/lib/AmpValidatorQuery.js

+    this.initRules_(rules);
+  }
+
+  getTagsForFormat(format, transformed) {


sebastianbenz · 2019-07-08T14:05:08Z

packages/validator-query/lib/AmpValidatorQuery.js

+        });
+  }
+
+  getExtensionsForFormat(format) {


I'd expect this method to return an array. Why return an object instead?

There shouldn't ever be more than one extension with the same name and...

...I assumed the query would usually be "give me information about amp-bind", so an object seems more appropriate. Also, it's cheap to transform an object back into an array with Object.keys and/or Object.values.

How about turning this into two different APIs?

const ampBindRule = validationRules.getExtension('amp-bind'); const extensions = validationRules.getExtensions() <- returns an array containing all extensions const mailExtensions = validationRules.getExtensions({format: 'AMP4EMAIL'}) <- with optional format parameter

I removed getExtensions and instead exposed it as an extensions property and added getExtension which follows your suggestion.

packages/validator-query/lib/AmpValidatorQuery.js

+ * limitations under the License.
+ */
+
+class AmpValidatorQuery {


fstanis · 2019-07-08T15:16:03Z

packages/validator-query/lib/AmpValidatorQuery.js

+    if (transformed) {
+      return this.checkEntityFormat_(entity, 'transformed');
+    }
+    if (entity.enabledBy && entity.enabledBy.includes('transformed')) {


Looking back, I wonder if I can just reuse checkEntityFormat_ and call it as !this.checkEntityFormat_(entity, 'transformed')...

Will need to give it some thought.

fstanis · 2019-07-09T17:26:38Z

All comments should be addressed.

What are typical use cases for API? Are these covered by the two methods we provide?

Hard to say: I went with a conservative API that I hope will evolve over time as we identify more use-cases.

Would it make sense to always fetch rules per format?

I'd be OK with that, but also don't see any advantages in that approach, especially considering the underlying data is the same for all formats.

Suggestion: can we rename the project to toolbox-validation-rules?

Done.

packages/validator-rules/README.md

+
+```javascript
+  // Loads the validator rules remotely with default options
+  const rules = validatorRules.fetch();


sebastianbenz · 2019-07-09T20:27:54Z

packages/validator-rules/README.md

+  console.log(tags.map(tag => tag.tagName));
+
+  // Get information about an extension
+  const ext = rules.getExtension('amp-carousel', 'AMP4EMAIL');


Why do you need to provide the format here? This seems counterintuitive.

This is just how validator rules are written - there's more than one extension definition with the same name, but different formats.

The implementation uses a different order getExtension(format, extension).

Ouch, good catch. Fixed.

packages/validator-rules/README.md

+
+Specifically:
+
+-   The `raw` property is unprocessed [ValidatorRules](https://github.com/ampproject/amphtml/blob/master/validator/validator.proto#L643)


packages/validator-rules/index.js

+let cached = null;
+
+async function load(opt) {
+  opt = opt || {};


fstanis · 2019-07-10T16:31:00Z

packages/validator-rules/index.js

+  }
+
+  let rules = opt.rules;
+  delete opt.rules;


I don't even remember why I had it there. Thanks for flagging, removed.

packages/validator-rules/README.md

+  console.log(tags.map(tag => tag.tagName));
+
+  // Get information about an extension
+  const ext = rules.getExtension('amp-carousel', 'AMP4EMAIL');


packages/validator-rules/lib/AmpValidatorRules.js

+            for (const attrList of tag.attrLists) {
+              tag.attrs.push(...this.attrLists_[attrList]);
+            }
+            delete tag.attrLists;


sebastianbenz · 2019-07-09T20:47:30Z

packages/validator-rules/lib/loadRules.js

+async function loadRules({source, url}) {
+  url = url || VALIDATOR_RULES_URL;
+  switch (source) {
+    case 'local':


Instead of configuring this via options, you could also provide to methods fetch and loadFile on the ValidationRules object.

I feel the functionality is not different enough to vouch having two different functions and that also prevents me from easily having the default option which is "try local if possible".

That said, maybe source is not needed if the url is a local file, i.e. use something like

amp-toolbox/packages/cli/lib/io.js

Line 25 in 533464d

async function loadUrlOrFile(urlOrPath) {

Ideally, whatever the resolution of #378 ends up being will replace this file anyway.

Sounds good. I'd suggest removing loadLocal until we settled on a common approach. Once it's out there it's harder to deprecate and I don't think that the feature is critical.

Fair point, done.

fstanis · 2019-07-10T16:34:57Z

Addressed all comments.

sebastianbenz

Looks good! Few nits.

sebastianbenz · 2019-07-10T17:29:04Z

packages/validator-rules/README.md

@@ -60,7 +61,7 @@ The rules used closely follow the proto definitions from [validator.proto](https

 Specifically:

-   The `raw` property is unprocessed [ValidatorRules](https://github.com/ampproject/amphtml/blob/master/validator/validator.proto#L643)
+-   The `raw` property is unprocessed [ValidatorRules](https://github.com/ampproject/amphtml/blob/master/validator/validator.proto#L643), the same format used by `https://cdn.ampproject.org/v0/validator.json`


nit: turn this into a link

Don't think that makes sense, clicking that link is probably not very useful, it's more meaningful as an identifier.

I'd want to click it :-). But I agree unformatted JSON is not helpful.

sebastianbenz · 2019-07-10T17:30:10Z

packages/validator-rules/lib/AmpValidatorRules.js

@@ -158,15 +158,21 @@ class AmpValidatorRules {
        .filter((tag) => !tag.extensionSpec)
        .map((tag) => {
          tag.attrs = tag.attrs || [];
+
+          // Merge global attribute lists into atrrs


nit: s/atrrs/attrLists

Your comment explains what you're doing, but I get that from the code. I'm more interested in the why (which I don't understand)

Tried rewriting. The key thing is that attrLists contains a list of IDs that are looked up from the top-level attrLists property. It doesn't contain any real attribute definition. This bit of code merges the real attribute definitions (from the global object) into attrs.

Let me know if the current comment makes sense to convey that.

sebastianbenz · 2019-07-10T17:33:47Z

packages/validator-rules/lib/loadRules.js

+async function loadRules({source, url}) {
+  url = url || VALIDATOR_RULES_URL;
+  switch (source) {
+    case 'local':


Sounds good. I'd suggest removing loadLocal until we settled on a common approach. Once it's out there it's harder to deprecate and I don't think that the feature is critical.

sebastianbenz

One nit

sebastianbenz · 2019-07-11T13:34:08Z

packages/validator-rules/README.md

+  console.log(tags.map(tag => tag.tagName));
+
+  // Get information about an extension
+  const ext = rules.getExtension('amp-carousel', 'AMP4EMAIL');


The implementation uses a different order getExtension(format, extension).

sebastianbenz · 2019-07-11T13:34:47Z

packages/validator-rules/README.md

@@ -60,7 +61,7 @@ The rules used closely follow the proto definitions from [validator.proto](https

 Specifically:

-   The `raw` property is unprocessed [ValidatorRules](https://github.com/ampproject/amphtml/blob/master/validator/validator.proto#L643)
+-   The `raw` property is unprocessed [ValidatorRules](https://github.com/ampproject/amphtml/blob/master/validator/validator.proto#L643), the same format used by `https://cdn.ampproject.org/v0/validator.json`


I'd want to click it :-). But I agree unformatted JSON is not helpful.

sebastianbenz · 2019-07-11T13:38:36Z

packages/validator-rules/lib/loadRules.js

+  return req.json();
+}
+
+async function loadRules({url}) {


I learned something, was't aware that restructuring works for params.

sebastianbenz · 2019-07-11T14:26:54Z

Whoop whoop! Thanks!

googlebot added the cla: yes label Jul 5, 2019

fstanis changed the title ~~Create amp-validatory-query package~~ Create amp-validator-query package Jul 5, 2019

fstanis requested review from ithinkihaveacat and sebastianbenz July 5, 2019 17:16

ithinkihaveacat requested changes Jul 8, 2019

View reviewed changes

sebastianbenz requested changes Jul 8, 2019

View reviewed changes

fstanis mentioned this pull request Jul 8, 2019

Revisit PHP representation of AMP validator spec ampproject/amp-wp#2769

Closed

fstanis changed the title ~~Create amp-validator-query package~~ Create amp-validator-rules package Jul 9, 2019

sebastianbenz reviewed Jul 9, 2019

View reviewed changes

sebastianbenz reviewed Jul 10, 2019

View reviewed changes

sebastianbenz approved these changes Jul 11, 2019

View reviewed changes

fstanis force-pushed the validator branch 2 times, most recently from d9e83d6 to 9d85829 Compare July 11, 2019 14:09

fstanis added 13 commits July 11, 2019 15:15

Add validator-query

da8e5fd

lint

c7ab597

add readme

f5214a8

fix lint and package name

a4cf89e

address comments

66757f0

use oneBehindFetch in rules

e02708d

rename from query to rules

84bc72d

address comments

a0178ce

rename extensions method

4807294

address comments

d1d9923

address comments

82481fa

fix README

9bcb263

update package-lock via lerna

e8e4dc1

fstanis force-pushed the validator branch from 9d85829 to e8e4dc1 Compare July 11, 2019 14:17

sebastianbenz merged commit 651f3d9 into ampproject:master Jul 11, 2019

		@@ -0,0 +1,57 @@
		# AMP-Toolbox Validator Query

		Makes it easier to query published AMP Validator rules and extract information


		Specifically:

		- The `raw` property is unprocessed [ValidatorRules](https://github.com/ampproject/amphtml/blob/master/validator/validator.proto#L643)

Create amp-validator-rules package #377

Create amp-validator-rules package #377

Conversation

fstanis commented Jul 5, 2019

ithinkihaveacat left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

sebastianbenz 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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

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

This comment was marked as resolved.

This comment was marked as outdated.

Choose a reason for hiding this comment

fstanis commented Jul 9, 2019

This comment was marked as resolved.

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

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

fstanis commented Jul 10, 2019

sebastianbenz 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

sebastianbenz 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

sebastianbenz commented Jul 11, 2019