GoogleChrome / GoogleChrome/lighthouse

Consider deprecating lighthouse-logger package

Open
#13,721 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

internals P3
Dominant language
JavaScript
Stars
30.8k
Forks
9.8k
Avg merge
1d 14h
Merged PRs (30d)
20

Description

It's kind of an awkward package to maintain.

And this is possibly unexpected: we bundle using the copy of lighthouse-logger we keep in this repo, but when running via Node we use the npm package.

copy paste chat log with paul:

Connor Clark
wdyt of removing our npm dep on lighthouse-logger
and just reference it directly in our code
https://knowyourmeme.com/memes/push-it-somewhere-else-patrick


Paul Irish, 49 min
and same w/ chromelauncher?

Connor Clark, 49 min
so I'm just looking at the bits remaining to convert to ESM
and this is a weird bit

Connor Clark, 49 min, Edited
beign not a npm package is easier

Paul Irish, 49 min
aye

Connor Clark, 49 min
so I wouldnt touch chrome launcher
basically the logger package on npm would probably just stay there forever

Paul Irish, 49 min
generally that seems fine but i feel like we need lighthouselogger as a singleton and so two impls wouldnt work?

Connor Clark, 48 min
and if we cared to we will update it ... but we never change it
hmm
let me see
We should be passing a logger reference around
if thats the case
not relying on two different packages to import the same log package to combine them
wait how does debug work?
is that singleton?
Ah, not a relevant question b/c our logger has state.
debug doesnt

Paul Irish, 46 min
also.. we could just .. use debug directly in CL
and maybe be fine with some mismatch

Connor Clark, 45 min
Do we even use the timings feature in CL?
Or just log?

Paul Irish, 45 min
just log

Connor Clark, 45 min
then I dont think them being different will matter

Paul Irish, 44 min


Connor Clark, 44 min
Oh! and anyway....all the state is kept in marky
so two diff instances of our logger....should just work ...

Paul Irish, 44 min
the setLevel call tho

Connor Clark, 44 min
it's all very confusing fr

Paul Irish, 43 min
yah we could just do this setLevel thing in CL directly with debug
and.. be fine

Connor Clark, 43 min
ok but that sets global state of debug

Paul Irish, 42 min
ya

Connor Clark, 42 min
isVerbose references the local level state
but that is never used?
not by us, maybe in CL?

Paul Irish, 42 min
unused

Connor Clark, 42 min
cool
i think we can do this

Paul Irish, 42 min
ya
++

Connor Clark, 40 min, Edited
ok last thing...what if different versions of debug are in play
meh.
moving CL to debug directly sounds nice

Paul Irish, 39 min
ya

Connor Clark, 24 min
oh, when we bundle we at least use the local instance
not npm
but CLI is the npm package

Paul Irish, 24 min
ah

branch is logger-local

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the logger-local branch and trace where the repository-local and npm copies of lighthouse-logger are used, including the CLI and bundling paths described here. Clarify whether logger state, setLevel, isVerbose, and marky timing behavior can be safely unified or removed, then document the decided deprecation boundary and validate both execution modes.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
cli, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.