-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
feat(stack-packs): added draft for stack packs #7243
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
talked this out with @patrickhulce ... Here's the proposal:
stack pack definition: stackPacks: [
{
id: 'wordpress'
title: 'WordPress',
iconDataUri: 'data:image/...',
requiredTechnologies: ['js:wordpressapi', 'core:wordpress'], // see 'type' and 'id' below.
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, ........',
'uses-webp-images': 'Consider using a plugin/service that will automatically convert your uploaded images to the optimal formats.',
// ...
}
}
} Technologies artifact interface GathererArtifacts {
// ...
Technologies: DetectedTechnology[]
}
interface DetectedTechnology {
type: 'js', // basically the source of the detection. js is js-library-detector..
// and 'core' is any detections custom defined in lighthouse's Technologies artifact.
id: string // if (type=='js') lib.npmPkgName || lib.icon.
// if (type=='core') whatever we think is decent.
name: string // the display-friendly name of the technology
version?: string // any version data we have
npm?: string // npmPkgName if it exists
} |
sounds good to me, let me cleanup this PR to get comments resolved |
Unless we're renaming these "TechPecks", it seems like it's confusing to call this anything but
|
What is |
Yeah it's changed to now be how it is detected, so +1 to renaming to |
K. That works for me.
another +1 for |
FYI : Submitted a pull request to include WordPress detection to the library-detector (since we still plan to use it for V1) |
3014960
to
d7802b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my blocking comments have been addressed, i.e. proto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some more clean-up ideas for some of the comments and the Stack
types, but the overall structures look good (and I took too long with my changes :).
I don't want to block #8154, etc, which are necessary for getting i18n going, so let's move forward. I fixed the cli snapshot test and we can merge when green, then proceed from there
🎉 🎉 |
just a warning for those using I've been deleting the |
does |
Summary
This is a draft of stack packs to show how the integration will look like and to discuss some pain points before we actually implement it.
I've added a stack-packs.js file inside lib that creates the lhr result for stackpacks. It detects these stack-packs from LH artifacts.
The draft design of the stack pack result looks like:
this would map to
note, this ended up as
A put up a small demo up:
http://wardpeet-filestorage.surge.sh/lh-store/pr-7243/report.html
Related Issues/PRs
#7021