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: update readme, add docs/readme, modernize a bit #2341

Merged
merged 8 commits into from
May 24, 2017
Merged

Docs: update readme, add docs/readme, modernize a bit #2341

merged 8 commits into from
May 24, 2017

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented May 23, 2017

  • Updates screenshots
  • moves main readme examples to docs/readme.md
  • adds a "turn on logging" example to the node snippet
  • Beefs up the custom audit recipe and removes dupe content from the readme
  • Adds info on DevTools integration
  • Adds recipe/doc links to main readme

readme.md Outdated
> Helpful for CI integration

- [gulp](docs/recipes/gulp)

## Viewing a report

Lighthouse can produce a report as JSON, HTML, or stdout CLI output.
Copy link
Member

Choose a reason for hiding this comment

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

no more stdout CLI. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. done

readme.md Outdated
## Develop

Also see [Contributing](./CONTRIBUTING.md) for more information.
Copy link
Member

Choose a reason for hiding this comment

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

Read on for the basics of hacking on Lighthouse. Also see...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

docs/readme.md Outdated
@@ -0,0 +1,109 @@
Useful documentation, examples, and [recipes](./recipes/) to get you started.
Copy link
Member

Choose a reason for hiding this comment

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

kinda think this should be overview.md or basics.md.

This kinda forces the user to choose a markdown file when entering /docs but i think that's good. it also means we don't have to maintain this file as a table-of-contents of all our docs

Copy link
Contributor Author

@ebidel ebidel May 23, 2017

Choose a reason for hiding this comment

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

I used readme.md for that reason :)...so people would see some of the most common examples when they hit /docs. It's not really a table of contents though. [recipes](./recipes/) will give them the list of directories/md files. /readme.md has a manual list so folks can search the main repo page and find drilled in content.

Copy link
Member

Choose a reason for hiding this comment

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

well... im okay with it being readme. but let's add one more link to architecture.md at the top or so.

@paulirish
Copy link
Member

These changes look great. Nice stuff. :)

docs/readme.md Outdated
@@ -0,0 +1,109 @@
Useful documentation, examples, and [recipes](./recipes/) to get you started.
Copy link
Member

Choose a reason for hiding this comment

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

well... im okay with it being readme. but let's add one more link to architecture.md at the top or so.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

a-ok by me, the readme was a bit of a monster :)

readme.md Outdated
To use Lighthouse from within the DevTools, open the tools, select the Audits panel,
and hit "Perform an Audit...".

<img width="350px" alt="Lighthouse integration in CHrome DevTools" src="https://cloud.githubusercontent.com/assets/238208/26366636/ada298f8-3fa0-11e7-9da5-ede2c906d10c.png">
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: s/CHrome/Chrome/ :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

lots of little things


function launchChromeAndRunLighthouse(url, flags, config = null) {
return chromeLauncher.launch().then(chrome => {
flags.port = chrome.port;
Copy link
Member

Choose a reason for hiding this comment

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

port ends up a distracting detail in basic usage. maybe we shouldn't have done a random port by default.. :)

Copy link
Member

Choose a reason for hiding this comment

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

i think it's worth it. but if you wanna discuss, let's open an issue?

Copy link
Member

@brendankenny brendankenny May 23, 2017

Choose a reason for hiding this comment

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

I don't know how much it actually hurts things or how much the gains matter, so not sure if it's worth opening an issue :)

It's just when I see examples like this it seems like one more requirement to remember without any advantages for basic usage

const log = require('lighthouse/lighthouse-core/lib/log');

const flags = {logLevel: 'info', output: 'json'};
log.setLevel(flags.logLevel);
Copy link
Member

Choose a reason for hiding this comment

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

log.setLevel(flags.logLevel); shouldn't need to be done by a user since the LH module does this based on flags.logLevel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you need it to see the chrome launcher's output.

Copy link
Member

Choose a reason for hiding this comment

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

you need it to see the chrome launcher's output

that seems like a chrome-launcher bug. I think we discussed removing log from there, but it should take a logLevel until/if it gets removed

docs/readme.md Outdated

## Testing on a site with authentication

Lighthouse adds `chrome-debug` to the CLI when you install it from npm.
Copy link
Member

Choose a reason for hiding this comment

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

npm/yarn?

Copy link
Member

@paulirish paulirish May 23, 2017

Choose a reason for hiding this comment

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

When installed globally via npm install -g or yarn global add, a chrome-debug binary is added, which will launch a standalone Chrome instance with a debugging port open.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

docs/readme.md Outdated

// Usage:
launchChromeAndRunLighthouse('https://example.com', flags)
.then(results => console.log(results));
Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer your previous comment version of this (// Use results or whatever) rather than just console.logging it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

docs/readme.md Outdated

## Lighthouse as trace processor

Lighthouse can be used to analyze trace and performance data collected from other tools (like WebPageTest and ChromeDriver). The `traces` and `devtoolsLogs` artifact items can be provided using a string for the absolute path on disk. The `devtoolsLogs` array is captured from the Network domain (a la ChromeDriver's [`enableNetwork` option](https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object)) and reformatted slightly. As an example, here's a trace-only run that's reporting on user timings and critical request chains:
Copy link
Member

Choose a reason for hiding this comment

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

devtoolsLogs is Page and Network now (though to be fair, this will continue to be weird to use until we get GAR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what are are the updates?

Copy link
Member

Choose a reason for hiding this comment

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

The devtoolsLogs array is captured from the Network domain

to

The devtoolsLogs array is captured from the Network and Page domains (a la ChromeDriver's [enableNetwork and enablePage options]

@@ -234,8 +178,6 @@ yarn build-all
# cd lighthouse-cli && yarn dev
Copy link
Member

Choose a reason for hiding this comment

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

does chrome-launcher need a tsc note too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

readme.md Outdated

Useful documentation, examples, and recipes to get you started.

**Docs**
Copy link
Member

Choose a reason for hiding this comment

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

should this be moved higher? Not as useful for first time visitors, but useful for repeat visitors. At least, when I need to get to a particular set of instructions I usually go to a project's readme and hope that it's linked to from there. Don't know what other people do :)

readme.md Outdated

## Related Projects

* [webpack-lighthouse-plugin](https://github.com/addyosmani/webpack-lighthouse-plugin) - run Lighthouse from a Webpack build.
Copy link
Member

Choose a reason for hiding this comment

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

irregular spacing here and below?

Copy link
Member

Choose a reason for hiding this comment

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

we should fix up the wording on these, too. "gather" vs "gathers", active vs passive, etc

readme.md Outdated
* [lighthouse-mocha-example](https://github.com/justinribeiro/lighthouse-mocha-example) - gathers performance metrics via Lighthouse and tests them in Mocha
* [pwmetrics](https://github.com/paulirish/pwmetrics/) - gather performance metrics
* [lighthouse-hue](https://github.com/ebidel/lighthouse-hue) - Lighthouse score setting the color of Philips Hue lights
* [lighthouse-batch](https://www.npmjs.com/package/lighthouse-batch) - Run Lighthouse over a number of sites in sequence and generating a summary report including all of their scores.
Copy link
Member

Choose a reason for hiding this comment

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

generating -> generate? Or Runs/generates. Depends on how the others end up

readme.md Outdated

Our session from Google I/O 2017: architecture, writing custom audits, Github/Travis/CI integration, and more:
> **Tip**: see [Lighthouse Architecture](./docs/architecture.md) more information
Copy link
Member

Choose a reason for hiding this comment

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

"see Lighthouse Architecture for information on terminology and architecture"?

readme.md Outdated
```

## Tests
### Tests

Some basic unit tests forked are in `/test` and run via mocha. eslint is also checked for style violations.
Copy link
Member

Choose a reason for hiding this comment

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

this sentence has morphed into something that makes no sense (and the "basic unit tests" doesn't apply anymore...lots of unit tests plus smoke tests now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


A hypothetical site measures the time from navigation start to when the page has initialized and the main search box is ready to be used. It saves that value in a global variable, `window.myLoadMetrics.searchableTime`.
> **Tip**: see [Lighthouse Architecture](../../../docs/architecture.md) information
Copy link
Member

Choose a reason for hiding this comment

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

for information

readme.md Outdated

Our session from Google I/O 2017: architecture, writing custom audits, Github/Travis/CI integration, and more:
> **Tip**: see [Lighthouse Architecture](./docs/architecture.md) more information
Copy link
Member

Choose a reason for hiding this comment

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

also needs for

output into the Lighthouse report. This example extends [Lighthouse's
default configuration](https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/config/default.js).

**Note**: when extending default.js, passes with the same name are merged together,
Copy link
Member

Choose a reason for hiding this comment

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

filename might be obscure. Maybe "when extending the default config"?

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Those last comments, but otherwise looks good! Big improvement

readme.md Outdated
* [webpack-lighthouse-plugin](https://github.com/addyosmani/webpack-lighthouse-plugin) - run Lighthouse from a Webpack build.
* [lighthouse-mocha-example](https://github.com/justinribeiro/lighthouse-mocha-example) - gather performance metrics via Lighthouse and tests them in Mocha
* [pwmetrics](https://github.com/paulirish/pwmetrics/) - gather performance metrics
* [lighthouse-hue](https://github.com/ebidel/lighthouse-hue) - uses a Lighthouse score to set the color of Philips Hue lights
Copy link
Member

Choose a reason for hiding this comment

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

s/uses/use to match the other descriptions

readme.md Outdated
* [pwmetrics](https://github.com/paulirish/pwmetrics/) - gather performance metrics
* [lighthouse-hue](https://github.com/ebidel/lighthouse-hue) - uses a Lighthouse score to set the color of Philips Hue lights
* [lighthouse-batch](https://www.npmjs.com/package/lighthouse-batch) - run Lighthouse over a number of sites and generate a summary of their metrics/scores.
* [lighthouse-cron](https://github.com/thearegee/lighthouse-cron) - Cron multiple batch Lighthouse audits and emit results for sending to remote server.
Copy link
Member

Choose a reason for hiding this comment

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

the start of this is hard to parse, but that's what was submitted so we can leave as is for now :)

@ebidel ebidel merged commit fb86d50 into master May 24, 2017
@ebidel ebidel deleted the docs branch May 24, 2017 00:29
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.

4 participants