Update to psi v5 #98

JuanMaRuiz · 2019-03-20T23:34:50Z

Relates to #97

- Updated README with new psi version - Updated tests: renamed files, updated assertions - Updated output to extract new data from psi API - Changed response.json fixture with the new output from psi API

- Reverted npm test script - Minor improvements

sindresorhus · 2019-06-03T07:57:04Z

❯ psi https://addyosmani.com

--------------------------------------------------------

Summary

URL:          addyosmani.com
Strategy:     mobile
Performance:  98

Field Data



Lab Data

First Contentful Paint                     | 1.0 s
First CPU Idle                             | 1.9 s
First Meaningful Paint                     | 1.0 s
Max Potential First Input Delay            | 160 ms
Speed Index                                | 3.0 s
Time to Interactive                        | 1.9 s

Opportunities

Eliminate render-blocking resources        | 78ms
Properly size images                       | 150ms
Reduce server response times (TTFB)        | 349ms

--------------------------------------------------------

I noticed there's no Field Data.

And there's an inconsistency between 160 ms and 78ms.

sindresorhus · 2019-06-03T07:58:14Z

lib/output.js

+
+  const rules = getRules(lighthouseResult.categories.performance.auditRefs, 'load-opportunities');
+
+  const oppRule = rules.filter(opportunityRule => {


oppRule is not a good variable name. Use opportunityRule.

You are right! Modified

sindresorhus · 2019-06-03T07:58:25Z

lib/output.js

+    return ruleDetails.overallSavingsMs !== undefined && ruleDetails.overallSavingsMs > 0 && ruleDetails.type === 'opportunity';
+  });
+
+  oppRule.forEach(rule => {


Use a for-of loop.

Better in that way. I didn't want to modify too much in this first PR.

sindresorhus · 2019-06-03T07:58:45Z

lib/output.js


 function getReporter(format) {
-  format = ['cli', 'json', 'tap'].indexOf(format) === -1 ? 'cli' : format;
-  return require(`./formats/${format}`);
+  const outputFormat = ['cli', 'json', 'tap'].indexOf(format) === -1 ? 'cli' : format;


You can use .includes() instead of .indexOf()

- Use for...of - Better constant name

JuanMaRuiz · 2019-06-03T16:02:55Z

❯ psi https://addyosmani.com

--------------------------------------------------------

Summary

URL:          addyosmani.com
Strategy:     mobile
Performance:  98

Field Data



Lab Data

First Contentful Paint                     | 1.0 s
First CPU Idle                             | 1.9 s
First Meaningful Paint                     | 1.0 s
Max Potential First Input Delay            | 160 ms
Speed Index                                | 3.0 s
Time to Interactive                        | 1.9 s

Opportunities

Eliminate render-blocking resources        | 78ms
Properly size images                       | 150ms
Reduce server response times (TTFB)        | 349ms

--------------------------------------------------------

I noticed there's no Field Data.

That's because there is nothing to show according to PageSpeed API. Maybe we could remove that part if there is no data or provide the same response

The message could be something like this:

And there's an inconsistency between 160 ms and 78ms.

That is because the API provides the displayValue so I don't need to use pretty-ms. Two approaches here:

Use the configuration option {verbose: true} from pretty-ms. We are going to have an inconsistency as well because verbose options will show 160 milliseconds instead of 160 ms
Remove whitespaces from displayValue string returned by the API.

What approach do you prefer?

sindresorhus · 2019-06-05T02:14:51Z

The message could be something like this:

Looks good, but I would make it white and not yellow. It's not a warning. It's a notice.

Remove whitespaces from displayValue string returned by the API.

👍

- Added message when empty Field Data section - Removed whitespace between number and metric on API results

JuanMaRuiz · 2019-06-05T15:36:39Z

The message could be something like this:

Looks good, but I would make it white and not yellow. It's not a warning. It's a notice.

Remove whitespaces from displayValue string returned by the API.

👍

JuanMaRuiz · 2019-06-05T15:38:44Z

Hi @sindresorhus I think all requested changes are done.

Best regards.

markusdosch · 2019-08-26T13:05:10Z

Any chance that this gets merged? It looks like this pull request is ready 😊

addyosmani · 2019-08-26T17:57:00Z

Thank you for all of your work on this, @JuanMaRuiz and @sindresorhus for dedicating time to a review. We really appreciate it.

I would like to have one more set of eyes on the PR from the PageSpeed Insights team and we're going to do a quick pass of it shortly.

exterkamp

Hi, I'm one of the developers from PSI. This PR looks good!

I have some minor nits, but this could land as-is and take on most of the feedback as follow-ups, since most are enhancements on top of this PR.

exterkamp · 2019-08-26T19:03:36Z

lib/output.js

-    const optimizedResourceURL = RESOURCE_URL + querystring.stringify({url: response.id, strategy: parameters.strategy});
+    const threshold = getThreshold(parameters.threshold);
+    const {lighthouseResult, loadingExperience, id} = response;
+    const optimizedResourceURL = `${RESOURCE_URL}${querystring.stringify({url: id, strategy: parameters.strategy})}`;


This is being deprecated as well, I would remove optimizedResourceURL and the flag that controls it.

exterkamp · 2019-08-26T20:16:40Z

lib/output.js

      threshold
    ));

    if (parameters.optimized) {
-      console.log('\nHere are your optimized images:', humanizeUrl(optimizedResourceURL));
+      console.log(`\nHere are your optimized images: ${humanizeUrl(optimizedResourceURL)}`);


Delete this as well I think.

exterkamp · 2019-08-26T20:19:24Z

lib/formats/cli.js

    '',
-    _.map(ruleSetResults.filter(x => x.value > 0), renderSection).join('\n'),
+    setTitle('Opportunities'),


I feel like this section should be elided if Opportunities is empty, or have some control text like Field Data, something like "No opportunities provided."

^ this looks a little off

Totally agree. Added message if no 'Opportunities' are provided.

exterkamp · 2019-08-26T20:44:07Z

lib/output.js

 const sortOn = require('sort-on');
 const humanizeUrl = require('humanize-url');
+const prettyMs = require('pretty-ms');

 const THRESHOLD = 70;
 const RESOURCE_URL = 'https://developers.google.com/speed/pagespeed/insights/optimizeContents?';


exterkamp · 2019-08-26T21:10:55Z

lib/output.js

-      const error = new Error(`Threshold of ${threshold} not met with score of ${response.ruleGroups.SPEED.score}`);
+    const score = reversePercentage(lighthouseResult.categories.performance.score);
+    if (score < threshold) {
+      const error = new Error(`Threshold of ${threshold} not met with score of ${score}`);
      error.noStack = true;
      throw error;
    }


Can delete param.download as well, since this endpoint is EOL.

exterkamp · 2019-08-26T21:42:44Z

lib/output.js

@@ -23,74 +23,102 @@ function overview(url, strategy, scores) {
  });

  ret.push({
-    label: 'Speed',
-    value: scores.SPEED.score
+    label: 'Performance',


This is i18n'd in the report under lighthouseResult > categories > performance > title but that is some future gold plating.

exterkamp · 2019-08-26T21:44:52Z

lib/output.js

 }

+const reversePercentage = num => num * 100;


TIL there is a word for this.

Suggested change

const reversePercentage = num => num * 100;

const convertToPercentum = num => num * 100;

Great TIL that word as well. Modified

exterkamp · 2019-08-26T21:51:13Z

lib/output.js

+    const {audits} = lighthouseResult;
+    ret.push({
+      label: audits[rule.id].title,
+      value: audits[rule.id].displayValue.replace(/\s/g, '')


Modifying LH display values can be dangerous since it's hard to confirm it looks good in all locales. But I think this is okay...

Should I change this?

lib/output.js

exterkamp · 2019-08-26T22:06:09Z

lib/output.js

-      statistics(response.pageStats),
-      ruleSetResults(response.formattedResults.ruleResults),
+      overview(humanizeUrl(id), parameters.strategy, lighthouseResult),
+      fieldData(loadingExperience.metrics),


The Field Data here is only page level field data, if there is no page level data this field will fall back to origin level data. This makes no distinction between the two. Example:

This site has only Origin level data

"Field Data" ~= origin in this case

I suggest pulling from originLoadingExperience.metrics instead so that it is always origin data, or pull both and mark each section, one as "Field Data (page)" and one as "Field Data (origin)".

I have tried to change loadingExperience by originLoadingExperience but it seems that originLoadingExperience.metrics is not always part of the response. What do you think it would be the best approach? Should I left loadingExperience or I should show both metrics as you recommend?

JuanMaRuiz · 2019-09-01T14:31:02Z

Many thanks @exterkamp for your review. I'll check all your comments and I'll fix them.

Best regards.

- Removed deprecated optimized and download options from output

… show proper data

Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>

JuanMaRuiz · 2019-09-02T16:37:14Z

Hi @addyosmani, @sindresorhus and @exterkamp I think I have applied all the changes requested (except this one. Please, if you have any chance, could you please take a look again to this PR?

Many thanks.

Best regards

addyosmani

LGTM

addyosmani · 2019-09-06T23:18:19Z

Thank you immensely for all of your work on the V5 update, @JuanMaRuiz. Updating has been a long time coming, and we couldn't have done this without all of your help.

After discussing the latest status in person with @exterkamp, we are going to go ahead with landing your PR. We appreciate you addressing the majority of feedback comments.

In terms of next-steps after landing this PR:

The PageSpeed Insights team (well, @exterkamp :P) has kindly offered to keep a closer eye on this package and maintaining API parity than I have since it was first put together. We will be transferring the repo from my personal account over to GoogleChromeLabs in the next week or two. @sindresorhus will of course retain his current privs on the repo too.
Once moved over, Shane is going to sanity check we're all good for a release and publish a new major version of psi to npm. We'll try (time willing) to get together some release notes as part of this.

Looking forward to this going out! Thanks everyone!

JuanMaRuiz · 2019-09-07T10:49:15Z

Thank you @addyosmani for let me participate in this project. I'm very happy to help and to see this PR merged.

I'll keep an eye on this (or the GoogleChromeLabs project) and I'll try to help in future improvements. While I was working in this PR I saw several things that could be improved but, because they weren't related with this update, I decided not to add that changes.

Thanks to @exterkamp and @sindresorhus for their comments that improved the quality of this PR (code and API).

Best regards!

JuanMaRuiz added 5 commits March 19, 2019 21:35

Update to page speed insights v5

66e6e3e

- Updated README with new psi version - Updated tests: renamed files, updated assertions - Updated output to extract new data from psi API - Changed response.json fixture with the new output from psi API

Updated travis configuration to use minimum node v8

786bebd

Added opportunities to json format

90e4d79

Minor changes:

59ece59

- Reverted npm test script - Minor improvements

Fixed lint errors

d3f9bc1

JuanMaRuiz added the enhancement label Mar 20, 2019

Minor changes: Minor improvements

032d0ce

JuanMaRuiz mentioned this pull request May 28, 2019

Update pagespeed insight to v5 #97

Closed

sindresorhus reviewed Jun 3, 2019

View reviewed changes

Added PR improvements

b2b962f

- Use for...of - Better constant name

User message and fix for visual style consistency

c034549

- Added message when empty Field Data section - Removed whitespace between number and metric on API results

JuanMaRuiz closed this Jun 5, 2019

JuanMaRuiz reopened this Jun 5, 2019

JuanMaRuiz mentioned this pull request Jun 6, 2019

Updated psi dependency and fix some package vulnerabilities jrcryer/grunt-pagespeed#30

Merged

exterkamp approved these changes Aug 26, 2019

View reviewed changes

JuanMaRuiz and others added 5 commits September 2, 2019 17:10

Removed Optimized and download flags

c91e58a

- Removed deprecated optimized and download options from output

Removed unnecessary item.label check and added TODOs to README

7fb2604

No opportunities provided. Added message, created generic function to…

5b44a1b

… show proper data

Update lib/output.js

5cb1bd2

Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>

Changed loadingExperience by originLoadingExperience

cc145fc

JuanMaRuiz added 2 commits September 2, 2019 18:53

Reverted originLoadingExperience.metrics to loadingExperience.metrics

77cfdec

Fixed xo error

11f871a

addyosmani approved these changes Sep 6, 2019

View reviewed changes

addyosmani merged commit 0c6c01f into master Sep 6, 2019

addyosmani deleted the feature-psi-v5 branch September 6, 2019 23:18

ghost mentioned this pull request Sep 10, 2019

npm still installs V2 #99

Closed

snyk-bot mentioned this pull request Mar 12, 2020

[Snyk] Fix for 1 vulnerabilities enterstudio/psi#8

Open

enterstudio mentioned this pull request Nov 28, 2023

[Snyk] Fix for 8 vulnerabilities enterstudio/psi#23

Open


		const rules = getRules(lighthouseResult.categories.performance.auditRefs, 'load-opportunities');

		const oppRule = rules.filter(opportunityRule => {

	const reversePercentage = num => num * 100;
	const convertToPercentum = num => num * 100;

Update to psi v5 #98

Update to psi v5 #98

Conversation

JuanMaRuiz commented Mar 20, 2019

sindresorhus commented Jun 3, 2019

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

JuanMaRuiz commented Jun 3, 2019 • edited Loading

sindresorhus commented Jun 5, 2019

JuanMaRuiz commented Jun 5, 2019

JuanMaRuiz commented Jun 5, 2019

markusdosch commented Aug 26, 2019

addyosmani commented Aug 26, 2019

exterkamp 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuanMaRuiz Sep 2, 2019 • edited Loading

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

JuanMaRuiz Sep 2, 2019 • edited Loading

Choose a reason for hiding this comment

JuanMaRuiz commented Sep 1, 2019

JuanMaRuiz commented Sep 2, 2019

addyosmani left a comment

Choose a reason for hiding this comment

addyosmani commented Sep 6, 2019

JuanMaRuiz commented Sep 7, 2019

JuanMaRuiz commented Jun 3, 2019 •

edited

Loading

JuanMaRuiz Sep 2, 2019 •

edited

Loading

JuanMaRuiz Sep 2, 2019 •

edited

Loading