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

Update pagespeed insight to v5 #97

Closed
JuanMaRuiz opened this issue Mar 13, 2019 · 12 comments
Closed

Update pagespeed insight to v5 #97

JuanMaRuiz opened this issue Mar 13, 2019 · 12 comments

Comments

@JuanMaRuiz
Copy link
Contributor

Hi @addyosmani as we discussed in the PR I have been exploring the possibility to migrate to pagespeed v5 and to add the new lighthouse report to psi tool. As you already know the response from pagessped v2 and pagessped v5 are different but I think it is possible to make the upgrade to the new version with not too much effort and adding the new lighthouse data

I don't know if you have any other initial idea about how the data should be shown but I think that we could show the same approach as Page Speed Insights Page:

Summary section (Current overview section)

  • URL
  • Strategy
  • Performance score (instead of Speed)
    (Accessibility score, Best practices, SEO and PWA could be added to the flag options)

Field section

  • FCP
  • FID

Lab Data (Current statistics section)

  • First Contentful Paint
  • Speed Index
  • Time to Interactive
  • First Meaningful Paint
  • First CPU Idle
  • Estimated Input Latency

Opportunities (Current rule set results)

  • List of opportunities found by lighthouse

Let me know what do you know about this idea.

Another question, I don't know what is the way of doing the contributions, should I work on my own fork of this repo? or should I create a new branch to start working in the update?

Thanks in advance.

Best regards.

JuanMa.

@sindresorhus
Copy link
Contributor

Another question, I don't know what is the way of doing the contributions, should I work on my own fork of this repo? or should I create a new branch to start working in the update?

Create a new branch and then do a PR from that branch when it's ready for review. Don't commit directly to master unless it's something minor.

@JuanMaRuiz
Copy link
Contributor Author

Another question, I don't know what is the way of doing the contributions, should I work on my own fork of this repo? or should I create a new branch to start working in the update?

Create a new branch and then do a PR from that branch when it's ready for review. Don't commit directly to master unless it's something minor.

Many thanks for the quick answer. More clear now.

@addyosmani
Copy link
Collaborator

First of all a big thanks for thinking about what a revision to support V5 would look like, @JuanMaRuiz. Also an extended thanks to @sindresorhus for taking time out to chime in here :) Always nice to see a friendly face.

The proposal you suggested aligns quite well with how I would have gone about trying to present a version of the new pagespeed insights report in a CLI.

If we wanted to go further we could potentially also give users the ability to specify which parts of the report they want. For example, I may only want Field data. Or just Lab data. As there's now quite a lot of information available in these reports, I can imagine some users only wanting a subset.

That said we could also start with something that just updates to V5 and think about configuration options at a later point. Wdyt?

@JuanMaRuiz
Copy link
Contributor Author

Hi! @addyosmani

That said we could also start with something that just updates to V5 and think about configuration options at a later point. Wdyt?

Totally agree with you. We could update to v5 showing a basic report and after that we could improve given to the user the ability of select the type of report he wants, maybe we could generate a report file like lighthouse does.

I'm going to start working in the update. In the mean time, do you think (@addyosmani , @sindresorhus ) that we should publish the new psi version on npm? That really adds value to the current users? The PR updating to v4 was merged but it wasn't generated/publish on npm.

@sindresorhus
Copy link
Contributor

I would update to Pagespeed v5 first and do a release then. After that, we can improve the output in follow-up releases. I don’t see value in releasing v4 as it’s deprecated and best practices there might be outdated.

@addyosmani
Copy link
Collaborator

SGTM :)

@JuanMaRuiz
Copy link
Contributor Author

Agree with that! Let's do the update. Many thanks both of you!

@JuanMaRuiz
Copy link
Contributor Author

JuanMaRuiz commented Mar 19, 2019

Hi! @addyosmani and @sindresorhus

I have updated to v5 and added the output data we discussed above. Here is how it looks like

psi-v5-cli

vs page speed report on the web

page-speed-insights

Although the code is not finished yet, what do you think about the output? Does it meet your expectations?

Best regards.


Update

humanize-url module is outdated. If we update this package to v2.0.0 tests fail because of the node v6/v8. I also get an error trying to run the tests using node v6 with the new googleapis module version but not with node v8.

I assume that we should update the minimum node version in travis configuration filet to v8, but could you confirm that?

I have created a new branch where you can check the PR changes previous to the PR.

JuanMa.

@JuanMaRuiz
Copy link
Contributor Author

Hi @addyosmani if you have any time, could you please take a look to the PR and tell me if my approach makes sense for you?

Thanks in advance.

@mathieuforest
Copy link

V4 will be deprecated at the end of the month.

@JuanMaRuiz
Copy link
Contributor Author

V4 will be deprecated at the end of the month.

Thanks for the advice @mathieuforest , we are working to update psi package to v5.

Regards

@sindresorhus
Copy link
Contributor

Fixed by #98

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

No branches or pull requests

4 participants