Skip to content

Add isAmp check to linter api#4403

Merged
tharders merged 2 commits intofuturefrom
feature/pixi-is-amp-check
Sep 1, 2020
Merged

Add isAmp check to linter api#4403
tharders merged 2 commits intofuturefrom
feature/pixi-is-amp-check

Conversation

@tharders
Copy link
Copy Markdown
Collaborator

The linter api now first checks if the page is a declared AMP page and only if that is true the linter checks are executed.

This PR also has the imported release schedule updated, which I did not really want to include here, but should do no harm.

return versionMap;
};

const isAmp = ($) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We also want to check if it imports .*\/v0.js

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In the amp-toolbox the "isAmp" implementation just checks for the amp attribute.
What is the purpose of this additional check?
Should it be added with "and" or with "or"?

If it is "or" I think it is a very weak check to see if a page looks like it should be AMP since the pattern could match scripts that are not AMP easily. I think it would be better to check for <style amp- and <script custom-element="amp-

If it should be "and" I don't really see the benefit of adding it. When a user hosts the amp runtime on his server and the name no longer matches he should get an error that the page is not considered valid AMP and not that the page is no AMP at all (while the rest of the page might literally scream "I am an AMP page" ;-)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Works for me.

@tharders tharders force-pushed the feature/pixi-is-amp-check branch from dba1d13 to cdc6c14 Compare September 1, 2020 06:01
@tharders tharders merged commit 29f44e0 into future Sep 1, 2020
@tharders tharders deleted the feature/pixi-is-amp-check branch September 1, 2020 13:31
robinvanopstal added a commit that referenced this pull request Sep 1, 2020
* future:
  📦 Update package-lock.json
  Add isAmp check to linter api (#4403)
  limit the number of significant figures on pixi lab data results (#4376)
  📦 Move amphtml-validator to prod deps (#4413)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants