Add px guide by sebastianbenz · Pull Request #1156 · ampproject/amp-toolbox

sebastianbenz · 2021-03-05T19:03:58Z

No description provided.

kristoferbaxter

Lots of great work here, thank you!

Left you a few nits and other items for the future.

kristoferbaxter · 2021-03-08T15:53:23Z

packages/page-experience/bin/cli.js

+const url = process.argv[2];
+
+(async () => {
+  const result = await new PageExperienceGuide().analyze(url);


Can't wait for top level await!

kristoferbaxter · 2021-03-08T15:54:29Z

packages/page-experience/lib/PageDataGatherer.js

+const parseFontfaces = require('./helpers/parseFontface');
+
+// Pixel 5 XL
+const DEFAULT_VIEWPORT = {


Nit: Is there a URL you could link to that lists dimensions for popular devices?

Additionally, do we need to provide a dpi value to puppeteer?

Afaik no DPI value needed as the default is 1. Not sure why a link would be helpful here.

kristoferbaxter · 2021-03-08T15:56:13Z

packages/page-experience/lib/PageDataGatherer.js

+ * Renders a page in Puppeteer and collects all data required for the page experience recommendations.
+ */
+class PageAnalyzer {
+  constructor(config = {}) {


Nit: Does the config parameter need types?

Added type info

kristoferbaxter · 2021-03-08T15:56:39Z

packages/page-experience/lib/PageDataGatherer.js

+    return await this.gatherPageData(page, remoteStyles);
+  }
+
+  async gatherPageData(page, remoteStyles) {


Nit: Do these params and the return need to be typed?

kristoferbaxter · 2021-03-08T15:57:30Z

packages/page-experience/lib/PageDataGatherer.js

+       * @return {Array<string>} a list of URLs
+       */
+      const collectFontPreloads = () => {
+        return Array.from(document.querySelectorAll('link[rel=preload][as=font]')).map(


Is there a separate check for invalid preloads somewhere? Perhaps a preload of a value used in a font-face declaration but not preloaded as a font?

Not yet, created an issue.

kristoferbaxter · 2021-03-08T16:01:03Z

packages/page-experience/lib/PageDataGatherer.js

+        !request.url().startsWith('https://cdn.ampproject.org')
+      ) {
+        // Only donwload AMP runtime scripts as they're need for layouting the page
+        // Once self-hosting is a thing we'll have to change this


Is there a flag for when the CLI tools know self-hosting is valid?

Not yet, will add later

kristoferbaxter · 2021-03-08T16:02:43Z

packages/page-experience/lib/PageExperienceGuide.js

+   * @private
+   */
+  async loadChecks() {
+    const jsFileExtension = '.js';


Does this work with the recently launched module mode?

This loads files from the filesystem which we control.

kristoferbaxter · 2021-03-08T16:04:42Z

packages/page-experience/lib/helpers/parseFontface.test.js

+}
+`;
+  parseFontface(styles, 'http://example.com');
+});


For completeness should there be a check with subset fonts?

kristoferbaxter · 2021-03-08T16:05:57Z

packages/page-experience/lib/helpers/parseFontfaceSrc.js

+const fontfaceSrcParser = require('css-font-face-src');
+
+/**
+ * Extracts the best fontface URL for preloading (woff2 > woff > ?).


Smart here: I was naively thinking the result would always be woff2 but this code correctly validates if there are other formats available to use.

kristoferbaxter · 2021-03-08T16:06:42Z

packages/page-experience/prettier.config.js

@@ -0,0 +1,39 @@
+/**
+ * Copyright 2019 The AMP HTML Authors. All Rights Reserved.


removed the file as no longer needed

* Fixed JSDocs * More tests

patrickkettner · 2021-03-12T13:11:13Z

packages/page-experience/README.md

+  const pageExperienceGuide = new PageExperienceGuide();
+  const result = pageExperienceGuide.analyze('https://amp.dev');
+  console.log('result')
+})


is there a reason this is wrapped in () but not invoked?

not at all...fixed

patrickkettner · 2021-03-12T13:13:58Z

packages/page-experience/lib/PageDataGatherer.js

+   */
+  async execute(url) {
+    if (!this.started) {
+      throw new Error('Puppeteer not running, please call `start` first.');


is there any reason you don't just call start?

You're right, but somehow I preferred the more explicit symmetry of calling start and shutdown. It's an internal API though so I don't really care...

patrickkettner · 2021-03-12T13:21:15Z

packages/page-experience/lib/PageDataGatherer.js

+      ) {
+        // Only donwload AMP runtime scripts as they're need for layouting the page
+        // Once self-hosting is a thing we'll have to change this
+        // TODO: investigate whether we could cache these locally


You could catch them https://theheadless.dev/posts/request-interception/, and reply with local filesystem?

that was my idea as well

patrickkettner · 2021-03-12T13:22:41Z

packages/page-experience/lib/PageDataGatherer.js

+      return request.continue();
+    });
+
+    // Collect external stylesheets from requests as we can't read them otherwise due to CORS


Is there a way to miss some stuff as a result of this? Any reason not to just disable CORS in rendering options?

That didn't work for me.

patrickkettner · 2021-03-12T13:32:16Z

packages/page-experience/lib/checks/fontPreloading.js

+    }
+    if (!pageData.fontPreloads.includes(fontface.mainSrc)) {
+      if (fontface.mainSrc.startsWith('https://fonts.gstatic.com')) {
+        result.warn({


would it make sense to extract these text blobs into an external file?

patrickkettner · 2021-03-12T13:32:40Z

packages/page-experience/lib/checks/fontDisplay.js

+
+    if (!fontface.fontDisplay) {
+      result.warn({
+        title: 'Improve LCP using `font-display: optional`',


Would it make sense to extract the warnings into an external file?

For translation?

patrickkettner · 2021-03-12T13:33:06Z

comments and nits, nothing blocking a LGTM though

sebastianbenz · 2021-03-12T15:05:06Z

Thanks for the reviews!

v2.8.0-canary.4

6226a79

sebastianbenz force-pushed the add-px-guide branch from 3682c73 to d6e9c49 Compare March 5, 2021 19:12

add page experience guide

8f7e159

sebastianbenz requested a review from patrickkettner March 5, 2021 19:16

sebastianbenz marked this pull request as ready for review March 5, 2021 19:19

sebastianbenz requested a review from kristoferbaxter March 5, 2021 19:23

kristoferbaxter reviewed Mar 8, 2021

View reviewed changes

This was referenced Mar 10, 2021

page-experience: check for invalid font preloads #1157

Open

page-experience: support self-hosted runtimes #1158

Open

Addressed comments

6ba0f47

* Fixed JSDocs * More tests

sebastianbenz force-pushed the add-px-guide branch from d6e9c49 to 6ba0f47 Compare March 10, 2021 21:48

sebastianbenz requested a review from kristoferbaxter March 10, 2021 21:48

patrickkettner approved these changes Mar 12, 2021

View reviewed changes

sebastianbenz added 2 commits March 12, 2021 15:27

fix code snippet

0a2dd1f

set mobile viewport

4a34bb5

sebastianbenz force-pushed the add-px-guide branch from ae1425c to 4a34bb5 Compare March 12, 2021 14:51

sebastianbenz merged commit e7c2d16 into main Mar 12, 2021

sebastianbenz deleted the add-px-guide branch March 12, 2021 15:05

sebastianbenz added the enhancement label Mar 12, 2021

		@@ -0,0 +1,39 @@
		/**
		* Copyright 2019 The AMP HTML Authors. All Rights Reserved.

Conversation

sebastianbenz commented Mar 5, 2021

Uh oh!

kristoferbaxter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickkettner commented Mar 12, 2021

Uh oh!

sebastianbenz commented Mar 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants