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

Stack Packs #7021

Closed
wardpeet opened this issue Jan 15, 2019 · 17 comments
Closed

Stack Packs #7021

wardpeet opened this issue Jan 15, 2019 · 17 comments

Comments

@wardpeet
Copy link
Collaborator

wardpeet commented Jan 15, 2019

Stack Packs

Stack Packs give us the ability to tailor audits descriptions for specific platforms (e.g. Wordpress). @housseindjirdeh already made a proof of concept in the stacks packs repo. This gives you an idea on how it might look like in lighthouse reports.

This issue is not tackling all questions/issues we will face when implementing stack packs but it should provide us with a solid foundation to serve these packs out of lighthouse core.

image

Problem

Problem 1: we need a way to detect which frameworks/products are being used so we can connect them to stack packs. For example if we look at a wordpress site we can look at <meta name="generator" content="WordPress 5.3" /> but lots of site remove this tag. So it's not the best thing to look at. A lot of requests on wordpress sites have wp-content inside or other wp- directives so we could probably use these metrics as a good indication. This example shows that it won't be possible to have a one way ticket to detect a product/platform so we need some kind of detection mechanism to turn packs on.

Problem 2: replacing or extending audit descriptions is our second problem. It boils down to the question: Where are we setting these strings in the lighthouse process (report or LH result)? Now in lighthouse everything is set inside the result. The report renderer consumes this result to render. Both the ICU identifier and locale descriptions are included in the json output so the renderer can show the text in the correct language. So the best option is to put these platform pack descriptions also in the lhr output This gives us the ability to easily ship it with LH API, if it only lives in the report it won't be so easy.

Proposed solutions

This is a rough idea on how to get the stack packs integrated.

The idea is to extend the current config with an option called packs or platformpacks: which is an array with the installed stack packs. We populate our default-config with the core packs so they are enabled by default. see plugins as an example. We can also hardcode these values in core if we don't want people to turn these packs off.

Having an option in the LH config to enable stack packs also gives us the ability to expose some kind of API. An index.js file that exports some functions we want to consume. Top of my head I'm thinking of 2 exports: 1 export is a detect function that is ran inside Lighthouse runner and gets the artifacts as a parameter.

(code is not correct but you get the idea)

export const detect = (artifacts) => {
 return artifacts.networkRecords.find(record=> record.url.includes('wp-content'))
}

The other export is the pack itself (a rough example can be found here)

export const advice = {
  "unused-css-rules": "Consider reducing, or switching, the number of [WordPress plugins](https://wordpress.org/plugins/) loading unused CSS in your page. To identify plugins that are adding extraneous CSS, try running [code coverage](https://developers.google.com/web/updates/2017/04/devtools-release-notes#coverage) in Chrome DevTools. You can identify the theme/plugin responsible from the URL of the stylesheet. Look out for plugins that have many stylesheets in the list which have a lot of red in code coverage. A plugin should only enqueue a stylesheet if it is actually used on the page."
}

We could also add more exports like image or whatever so an icon can be displayed inside the report.

To get this data inside the LHR I propose adding an extra field (stackPacks) to an audit result which hold custom platform pack information. This way we can loop through this array to show extra fields in the report. It's probably also a good idea to store the information of enabled packs somewhere in LHR so we can display these values somewhere nicely as well.

{
   "id":"unused-css-rules",
   "title":"Defer unused CSS",
   "description":"Remove unused rules from stylesheets to reduce unnecessary bytes consumed by network activity. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/unused-css).",
   "stackPacks": [
         {
            "name": "wordpress"
            "description": "Consider reducing, or switching, the number of [WordPress plugins](https://wordpress.org/plugins/) loading unused CSS in your page. To identify plugins that are adding extraneous CSS, try running [code coverage](https://developers.google.com/web/updates/2017/04/devtools-release-notes#coverage) in Chrome DevTools. You can identify the theme/plugin responsible from the URL of the stylesheet. Look out for plugins that have many stylesheets in the list which have a lot of red in code coverage. A plugin should only enqueue a stylesheet if it is actually used on the page."
         }
    ],
   "score":1,
   "scoreDisplayMode":"numeric",
   "rawValue":0,
   "displayValue":"Potential savings of 8 KB",
   ...
}

I'm hoping this actually make sense and love some feedback on this.

@housseindjirdeh
Copy link
Contributor

housseindjirdeh commented Jan 15, 2019

Thanks for kicking things off @wardpeet :)

For problem 1: The js-libraries audit was merged in recently that uses Library-Detector-for-Chrome under the hood. I wonder if we can just piggyback off of this and detect for WordPress upstream to make things easier.

@wardpeet
Copy link
Collaborator Author

Sure we can for wordpress and major frameworks but we probably want something customizable to support 3th party packs too. I'm not sure if things like Shopify, Gatsby, Drupal,, ... will live in js-libraries. Even if we branch of, is it something we want to maintain? Or rather have the check be part of the advice pack itself.

@patrickhulce
Copy link
Collaborator

Nice! Lots of exciting stuff in here!

A couple clarifying questions:

  1. Do we intend to support multiple detected platforms at once?
  2. Are platform packs always additive? i.e. Do we always keep the original description?

A few thoughts:

  • For problem 1, like @housseindjirdeh, I'd also love to see all our detection be part of our standard artifacts so it can be reused in other audits/plugins. We'd have to think this through in combination with our js-libraries as he already pointed out, but I'm excited about this as one of most broadly impactful pieces of platform packs :D (This probably goes hand-in-hand with Gatherer/artifact cleanup #6747)
  • For problem 2, I think the trickiest part of getting this in the LHR is the icon. A couple options I see in increasing order of attractiveness to me:
    1. Reference some pre-known enum like icon: 'wordpress' that the renderer bundles
    2. Create some object in LHR similar to rendererFormattedStrings that contains data URIs with these icons.
    3. Maintain some set of icons on a CDN and just reference their URL.
  • For proposed solution, depending on our success moving detection into core, the pack really just becomes the strings to replace and the icon reference.
  • Probably too early for this sort of feedback buuuuuuuut by the time it reaches the audit level, I'm not sure platformPacks is the right word since the "pack" part was the bundle of messages :) platformAdvice maybe?

That's all I've got for now, but looks great platform packs are in good hands 😃

@housseindjirdeh
Copy link
Contributor

To add on to the clarifying questions:

  1. Have we decided how to surface platform strings? Are we going to immediately show additional strings with logos if a platform is detected or provide a toggle of some sort? I plan on asking other folks to get their input but can imagine the latter will make things more complicated.

@addyosmani
Copy link
Member

We've decided to rename "Platform Packs" to "Stack Packs". This shouldn't really change anything much about the way this feature has been getting developed, but wanted to share :)

@wardpeet wardpeet changed the title Platform packs Stack Packs Feb 1, 2019
@patrickhulce
Copy link
Collaborator

Oooooo, I love it! Now users can go on a stack pack hack attack to fix up their perf :D

@addyosmani
Copy link
Member

Oooooo, I love it! Now users can go on a stack pack hack attack to fix up their perf :D

The t-shirt writes itself :D

@wardpeet
Copy link
Collaborator Author

wardpeet commented Feb 5, 2019

  1. Do we intend to support multiple detected platforms at once?

I think we do but probably @housseindjirdeh and @kusler can answer this correctly.

  1. Are platform packs always additive? i.e. Do we always keep the original description?

same as number 1 😋

  • For problem 1, like @housseindjirdeh, I'd also love to see all our detection be part of our standard artifacts so it can be reused in other audits/plugins. We'd have to think this through in combination with our js-libraries as he already pointed out, but I'm excited about this as one of most broadly impactful pieces of platform packs :D (This probably goes hand-in-hand with Gatherer/artifact cleanup #6747)

This would only work if all stack-packs are living in lighthouse (or at least are maintained by us, together with the stack detector), I think it's a good way to start but we have to drop the js-libraries name 😄 .

  • For problem 2, I think the trickiest part of getting this in the LHR is the icon. A couple options I see in increasing order of attractiveness to me:
  • Create some object in LHR similar to rendererFormattedStrings that contains data URIs with these icons.

I believe data-uris is probably what we want to do as we want our report to be accessible offline. We might want to put a limit on these uris.

  • For proposed solution, depending on our success moving detection into core, the pack really just becomes the strings to replace and the icon reference.

@patrickhulce do you mean that it would just be part of the lhr json?

@patrickhulce
Copy link
Collaborator

This would only work if all stack-packs are living in lighthouse

I like the idea of expanding our library detection such that we have the right artifacts to detect anyone big enough to care about writing a stack pack. Good motivation to keep improving the quality of our artifacts :)

I think it's a good way to start but we have to drop the js-libraries name 😄

Sounds good! I'm not attached to it, let's make it a first class artifact for plugins and packs!

@patrickhulce
Copy link
Collaborator

  • For proposed solution, depending on our success moving detection into core, the pack really just becomes the strings to replace and the icon reference.
    do you mean that it would just be part of the lhr json?

I was referring to the fact that the first iteration of stack packs could potentially be implementation-less and be purely data if we structured the library detection artifact the right way. No matter what the advice is going to have to be put into the LHR JSON, right?

Potential example "implementation" of a stack pack

{
  // seems like they need an ID of somesort 🤷‍♂️ 
  "id": "react",
  // icon as data URI as discussed
  "icon": "data:...",
  // The list of libraries that, if any detected, trigger the display of stack pack advice
  // If things get more complicated this could be a function that accepts all artifacts or something
  "detectedLibraries": ["react", "react-dom"],
  // The advice itself
  "advice": {
    "uses-responsive-images": {...}
  }
}

@wardpeet
Copy link
Collaborator Author

wardpeet commented Feb 5, 2019

oh yeah I like the idea to have a separate section of stackpacks and than let the renderer to it's thing. I was on the level of integrating it inside the audit section but this is way better!

{
  "audits": ...,
  "i18n": ...,
  "stackpacks": [
    {
      // seems like they need an ID of somesort 🤷‍♂️ 
      "id": "react",
      // icon as data URI as discussed
      "icon": "data:...",
      // The list of libraries that, if any detected, trigger the display of stack pack advice
      // If things get more complicated this could be a function that accepts all artifacts or something
      "detectedLibraries": ["react", "react-dom"],
      // The advice itself
      "advice": {
        "uses-responsive-images": {...}
      }
    }
  ]
}

I think it's a good way to start but we have to drop the js-libraries name 😄
Sounds good! I'm not attached to it, let's make it a first class artifact for plugins and packs!

The reason why I don't like js-libraries is that it doesn't fit the purpose anymore, wordpress isn't a js library so why would it detect it 😄 . Our detector might use js-libraries to handle js libs like react, ....

@paulirish paulirish reopened this Feb 5, 2019
@paulirish
Copy link
Member

(Oops.)

detection

We don't yet know if WordPress can be reliably detected with JS. I filed GoogleChrome/lighthouse-stack-packs#8 to get a more definitive answer on it.

I think it'd be super nice if JS-Lib-Detector was our source of truth for all these, but I'm also a little skeptical that clientside JS will always provide an indication for the stack packs that will be built. Patrick proposes we bind to js-lib-detector for v1, which seems fine enough.

So let's hope that WordPress is good with a JS detect and our first other customers are, too. ;)

multiple packs

Do we intend to support multiple detected platforms at once?

yes.

Are platform packs always additive? i.e. Do we always keep the original description?

yes. keep orig description and add the packs beneath.

pack definition and LHR

Patrick's strawman for the plugin export lgtm.

Ward's proposal of adding them as a toplevel array to the LHR also sgtm. (Seems nicer than trying to smush into LHR.audits) They don't need the detectedLibraries prop when they're in the LHR, but the other bits might as well remain.

@patrickhulce
Copy link
Collaborator

The reason why I don't like js-libraries is that it doesn't fit the purpose anymore, wordpress isn't a js library so why would it detect it

Yeah very good points. What I've been unsuccessfully trying to suggest so far is that we refactor the js-lib-detector artifact into a proper DetectedTechnologies artifact, or something, that can act as our single source of truth for all detected stacks/js libraries/etc.

Agreed with Paul for now I was hoping we can just use it straight up, but I'm all on board with a proper 1st class artifact that wraps up the detection logic.

@brendankenny
Copy link
Member

brendankenny commented Feb 5, 2019

  • what's the form of the advice objects? For v1 would it be sufficient to have a single string (description like in the audit result?) that's shown if the audit fails?

  • how dynamic should it be? Should advice contain all those objects, regardless of the pass/fail state of the audits, then it's purely up to the report renderer decides whether or not to display them?

Having it not dynamic would be nice, though maybe it would end up too limited or too large in the LHR. All the core would be responsible for at that point would be to copy the advice pack into the LHR if a particular stack was detected (plus maybe some i18n, if we get to that point with the stack packs)

@paulirish
Copy link
Member

how dynamic should it be?

IMO not at all. we always show it regardless of pass/fail.

what's the form of the advice objects? For v1 would it be sufficient to have a single string

sgtm. like das?

  // The advice itself
  "advice": {
    "total-byte-weight": "Consider showing excerpts in your post lists (e.g. via the more tag), reducing the number of posts shown on a given page, breaking your long posts into multiple pages, or using a plugin to lazy-load comments.",
    "render-blocking-resources": "There a number of WordPress plugins that can help you inline critical assets or defer less important resources. Beware that optimizations provided by these plugins may break features of your theme or plugins, so you will likely need to make code changes.",
  },

@wardpeet
Copy link
Collaborator Author

@paulirish was indeed what I was thinking of too.

Yeah very good points. What I've been unsuccessfully trying to suggest so far is that we refactor the js-lib-detector artifact into a proper DetectedTechnologies artifact, or something, that can act as our single source of truth for all detected stacks/js libraries/etc

This was my point to that we want one artifact and call it something proper and not js libraries 😄 but it's okay for v1. Putting up a PR this evening.

@brendankenny
Copy link
Member

This seems done. Let's call it done :)

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

6 participants