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

extract logger from lighthouse #2528

Merged
merged 1 commit into from
Jun 20, 2017
Merged

extract logger from lighthouse #2528

merged 1 commit into from
Jun 20, 2017

Conversation

samccone
Copy link
Contributor

Fixes #2526

@samccone samccone requested a review from paulirish June 18, 2017 19:47
@samccone samccone force-pushed the extract-logger branch 4 times, most recently from f6f9bf5 to 7c74318 Compare June 18, 2017 20:17
@@ -13,7 +13,7 @@ import {DEFAULT_FLAGS} from './flags';
import {makeTmpDir, defaults, delay} from './utils';
import * as net from 'net';
const rimraf = require('rimraf');
const log = require('lighthouse/lighthouse-core/lib/log');
const log = require('lighthouse-logger');
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh boy haha, can we just grab the lighthouse org instead? 😃

  • require('@lighthouse/logger')
  • require('@lighthouse/chrome-launcher')
  • require('@lighthouse/all-the-things')

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://www.npmjs.com/org/lighthouse looks like it's already taken, but no one is using it...

Copy link
Collaborator

@wardpeet wardpeet Jun 19, 2017

Choose a reason for hiding this comment

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

maybe we should use @____lighthouse as an org 😛 I believe it would 🚀 our npm installs as it's so easy to type (#sarcasm)

@@ -83,7 +83,6 @@ gulp.task('browserify', () => {
removeComments: true
})
.transform('brfs')
.ignore('../lighthouse-core/lib/log.js')
Copy link
Contributor

Choose a reason for hiding this comment

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

think viewer still wants to omit logger from the brfs output.

.ignore('../lighthouse-logger/index.js')?

Copy link
Member

Choose a reason for hiding this comment

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

think viewer still wants to omit logger from the brfs output.

FWIW, this is just a bundle-size optimization that won't matter after viewer switches to LH2 in #2521

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated per @ebidel's comment

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

tested CLI flags and extension logging. they looking good.

question from Eric stands, though

version "6.22.0"
resolved "https://registry.yarnpkg.com/babel-code-frame/-/babel-code-frame-6.22.0.tgz#027620bee567a88c32561574e7fd0801d33118e4"
dependencies:
chalk "^1.1.0"
esutils "^2.0.2"
js-tokens "^3.0.0"

babel-core@^6.0.2, babel-core@^6.23.0:
Copy link
Member

Choose a reason for hiding this comment

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

yargs and babel-core removed from this lockfile. I guess it was just stale?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed

@@ -632,6 +632,12 @@ debug@2.2.0, debug@^2.1.1, debug@^2.2.0:
dependencies:
ms "0.7.1"

debug@^2.6.8:
Copy link
Member

Choose a reason for hiding this comment

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

you're bumping to latest debug with this change, yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

@samccone
Copy link
Contributor Author

Thanks y'all

paulirish pushed a commit to GoogleChrome/chrome-launcher that referenced this pull request Aug 29, 2017
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.

7 participants