Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/linter optimizer checks #901

Merged
merged 4 commits into from
Aug 18, 2020
Merged

Conversation

tharders
Copy link
Collaborator

Implementing linter page experience checks for

  • Page is transformed AMP
  • Page is using Java Script Module version of the AMP Runtime.

@tharders
Copy link
Collaborator Author

I wonder if we should somehow add the recommendation to the linter rule (for example in the info field).
Otherwise it might not be clear what to do when running the tests in the command line.
(It looks like the "info" is currently not printed for results in the command line)

Copy link
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

✅ Few nits. But otherwise looks good.


export class IsTransformedAmp extends Rule {
run({ $ }: Context) {
const isTransformed = $("html[transformed='self;v=1']").length > 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Checking for the presence of transformed is enough. The content might change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


export class ModuleRuntimeUsed extends Rule {
run({ $ }: Context) {
const isTransformed = $("html[transformed='self;v=1']").length > 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might make sense to extract this into a helper method. We'll need it more often.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

return this.pass();
}
const isModuleVersion =
$("script[type='module'][src='https://cdn.ampproject.org/v0.mjs']")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just check for /v0.mjs to support runtime self-hosting in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

return isModuleVersion
? this.pass()
: this.warn(
"The Java Script module version of the AMP Runtime is not used"
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/Java Script/JavaScript

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@tharders tharders requested a review from sebastianbenz August 18, 2020 14:48
@sebastianbenz sebastianbenz merged commit fb51dc3 into main Aug 18, 2020
@sebastianbenz sebastianbenz deleted the feature/linter-optimizer-checks branch August 18, 2020 15:00
@sebastianbenz
Copy link
Collaborator

Build failure is unrelated

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

Successfully merging this pull request may close these issues.

3 participants