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

Add px guide #1156

Merged
merged 5 commits into from
Mar 12, 2021
Merged

Add px guide #1156

merged 5 commits into from
Mar 12, 2021

Conversation

sebastianbenz
Copy link
Collaborator

No description provided.

@sebastianbenz sebastianbenz marked this pull request as ready for review March 5, 2021 19:19
Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Lots of great work here, thank you!

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

const url = process.argv[2];

(async () => {
const result = await new PageExperienceGuide().analyze(url);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't wait for top level await!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yes...

const parseFontfaces = require('./helpers/parseFontface');

// Pixel 5 XL
const DEFAULT_VIEWPORT = {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Nit: Does the config parameter need types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added type info

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

async gatherPageData(page, remoteStyles) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not yet, created an issue.

!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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not yet, will add later

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

Choose a reason for hiding this comment

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

Does this work with the recently launched module mode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This loads files from the filesystem which we control.

}
`;
parseFontface(styles, 'http://example.com');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

For completeness should there be a check with subset fonts?

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

const fontfaceSrcParser = require('css-font-face-src');

/**
* Extracts the best fontface URL for preloading (woff2 > woff > ?).
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

2021?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed the file as no longer needed

* Fixed JSDocs
* More tests
const pageExperienceGuide = new PageExperienceGuide();
const result = pageExperienceGuide.analyze('https://amp.dev');
console.log('result')
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not at all...fixed

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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...

) {
// 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that was my idea as well

return request.continue();
});

// Collect external stylesheets from requests as we can't read them otherwise due to CORS
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That didn't work for me.

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

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For translation?

@patrickkettner
Copy link
Collaborator

comments and nits, nothing blocking a LGTM though

@sebastianbenz
Copy link
Collaborator Author

Thanks for the reviews!

@sebastianbenz sebastianbenz merged commit e7c2d16 into main Mar 12, 2021
@sebastianbenz sebastianbenz deleted the add-px-guide branch March 12, 2021 15:05
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