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

docs: add plugin recipe #6965

Merged
merged 11 commits into from
Mar 21, 2019
Merged

docs: add plugin recipe #6965

merged 11 commits into from
Mar 21, 2019

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Jan 9, 2019

broken out from #6959 since not a whole lot of effort was put into making a nice instructive recipe and it was just making that PR longer :)

Not really ready for review, but completely open for suggestions to make it better and just for reference.

@brendankenny brendankenny changed the title WIP: docs: add plugin recipe docs: add plugin recipe Feb 14, 2019
@brendankenny
Copy link
Member Author

Updated the recipe to something better. Happy to bikeshed.

Docs will be in a separate PR.

title: 'My Plugin Category',
description: 'Results for our new plugin category.',
auditRefs: [
{id: 'searchable-audit', weight: 1},
Copy link
Member

Choose a reason for hiding this comment

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

change to 'preload-as' now? (or is it preload-as-audit?)

Copy link
Member Author

Choose a reason for hiding this comment

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

or is it preload-as-audit

technically preload-as since that's what the audit's id is, but that's confusing, so I'll use preload-as-audit as the id :)

*/
'use strict';

const Audit = require('../../../../lighthouse-core/index.js').Audit;
Copy link
Member

Choose a reason for hiding this comment

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

a plugin would usually be written outside of our repo.. so wanna put the typical require path here?

Copy link
Member Author

Choose a reason for hiding this comment

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

a plugin would usually be written outside of our repo.. so wanna put the typical require path here?

yeah, I'll switch it. We do want it runnable (I was thinking about adding a recipes smokehouse target), but we can worry about that later

module.exports = {
// Additional audit to run on information Lighthouse gathered.
audits: [{
path: 'lighthouse-plugin-searchable/searchable-audit.js',
Copy link
Member

Choose a reason for hiding this comment

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

path

{
"name": "lighthouse-plugin-preload-as",
"private": true,
"main": "./plugin-config.js"
Copy link
Member

Choose a reason for hiding this comment

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

recipe-custom-config.js

Copy link
Member Author

Choose a reason for hiding this comment

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

recipe-custom-config.js

oh, this one is correct. The custom config is what you'd use with --config-path to run lighthouse with the plugin. The plugin's main is the actual plugin config file.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm.. weird to have two things that work together both called a config. about how index.js or plugin-index.js? plugin-dfn.js...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm.. weird to have two things that work together both called a config. about how index.js or plugin-index.js? plugin-dfn.js...

I'd actually rather have it stay as plugin-config.js and (eventually) get rid of the other one in this recipe.

Most of the time you'll either be writing a plugin or using a plugin in your config, not writing both at the same time, so it shouldn't cause confusion, and it makes sense to keep calling it a "plugin config" because it's pretty much identical to the main config except more locked down.

We need to add a --plugin cli flag, but at that point you won't need the recipe-custom-config.js file, which should clear up the confusion here.

Copy link
Member

Choose a reason for hiding this comment

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

a "plugin config" because it's pretty much identical to the main config except more locked down.

ehhh. IMO the Lighthouse concept of 'config' is very defined. And I think it'll be more confusing to use the term for two things that are similar but slightly different. It's not like our docs for plugins should say "go look at Lighthouse config, but only these parts."

Looking over at eslint plugins, they basically just call this a 'plugin'. No 'definition' or 'config' term. That sgtm. I still favor index.js but plugin.js seems ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

but plugin.js seems ok.

sure

@brendankenny
Copy link
Member Author

lol, amateur hour. Thanks.

@brendankenny
Copy link
Member Author

screen shot 2019-02-16 at 11 56 31

{
"name": "lighthouse-plugin-preload-as",
"private": true,
"main": "./plugin-config.js"
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm.. weird to have two things that work together both called a config. about how index.js or plugin-index.js? plugin-dfn.js...

module.exports = {
// Additional audit to run on information Lighthouse gathered.
audits: [{
path: 'lighthouse-plugin-preload-as/preload-as-audit.js',
Copy link
Member

Choose a reason for hiding this comment

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

interesting that the folder is included here even when the file is a sibling to this plugin definition file.

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting that the folder is included here even when the file is a sibling to this plugin definition file.

yeah, I wasn't sure how much magic to do on path resolution. We could have plugin folders added to the list of places that lighthouse looks for audits, or automatically prepend the plugin name to the path, but that might get messy quickly (e.g. what if the audit file you want to reference isn't in the plugin directory?).

OTOH, yeah, you have to write these paths relative to where Lighthouse looks, which is node_modules/, cwd, and then the path of the config file, which is kind of confusing in the simple case where you want to reference the audit right next to this file

class LoadAudit extends Audit {
static get meta() {
return {
id: 'preload-as-audit',
Copy link
Member

Choose a reason for hiding this comment

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

seems weird to put 'audit' in the id. how about 'preload-as' as the id and preload-as.audit.js as filename?

Copy link
Member Author

Choose a reason for hiding this comment

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

seems weird to put 'audit' in the id. how about 'preload-as' as the id and preload-as.audit.js as filename?

I'd agree with you if this was one of our audits, but it's helpful to have it in the filename, and it seems confusing to me to not have it match the id at that point

Copy link
Member

@paulirish paulirish Mar 1, 2019

Choose a reason for hiding this comment

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

preload-as.audit.js and id: 'preload-as', ! everybody wins.

or move the audit js file into /audits/preload-as.js.. that seems nice, too.

Copy link
Member Author

@brendankenny brendankenny Mar 1, 2019

Choose a reason for hiding this comment

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

everybody wins

that's the opposite of what I said :P

or move the audit js file into /audits/preload-as.js

adding a level of indirection seems worse for an example

Copy link
Member

@exterkamp exterkamp Mar 21, 2019

Choose a reason for hiding this comment

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

I'm definitely in the preload-as-audit is gross for including "audit". I personally like having a directory structure, to me it kind of keeps things all in their place. I like audits/preload-as.js, it gives the plugins a more common form of plugin.js & audits/<your-audits>.js being the standard parts.

@brendankenny
Copy link
Member Author

updated to use --plugins instead of another confusing config file and added a brief readme

"name": "plugin-lighthouse-recipe",
"private": true,
"dependencies": {
"lighthouse": "^4.3.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

intentional as --plugins landed after 4.2.0 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

intentional as --plugins landed after 4.2.0 :)

actually, jk, that's just really broken. Instead, now it just won't run the plugin since --plugins won't be recognized, but it won't throw an error. The release of 4.3.0 and semver (and/or we remember to update the recipe) will make it work when that comes out

@@ -0,0 +1,7 @@
{
"name": "plugin-lighthouse-recipe",
Copy link
Member

Choose a reason for hiding this comment

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

hmm. what does this package.json do? do we need it?

Copy link
Member Author

Choose a reason for hiding this comment

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

what does this package.json do?

it lets you run the recipe

Copy link
Member

Choose a reason for hiding this comment

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

ah. it's a little confusing to me with two manifests over here. Like.. for a user.. do they need both?

the plugin folder doesnt have a readme, but anyone making a plugin would be placing a readme in there.

so i'm thinking we just move this readme a level deeper and drop this package.json

Copy link
Member Author

Choose a reason for hiding this comment

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

so i'm thinking we just move this readme a level deeper and drop this package.json

this makes sense, so here it is, but it basically isn't runnable now

docs/recipes/plugin/readme.md Outdated Show resolved Hide resolved
docs/recipes/plugin/readme.md Outdated Show resolved Hide resolved
@GoogleChrome GoogleChrome deleted a comment from paulirish Mar 21, 2019
# Lighthouse plugin recipe

## Contents
- `lighthouse-plugin-preload-as` - the plugin module
Copy link
Member

Choose a reason for hiding this comment

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

Another thing I'm noticing... the Plugin is really about describing the category, not one of the two audits within it.

This category is titled 'My Plugin Category'... but let's pretend it was called 'Example Plugin'. I would expect the folder it's in to match the name of the category.. a la lighthouse-plugin-example-plugin

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed lighthouse-plugin-example


## To run

- Install somehow.
Copy link
Member

Choose a reason for hiding this comment

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

we could nuke private and publish this. sets the convention of folks publishing with the lighthouse-plugin- prefix.

or w/e. i'm chill.

@paulirish paulirish merged commit 0cd2214 into master Mar 21, 2019
@paulirish paulirish deleted the pluginrecipe branch March 21, 2019 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants