misc(logger): namespace debug with lh:

[patrickhulce]

Summary
this gets the ball rolling on #5683, the primary wrinkle is ChromeLauncher, which reuses lighthouse-logger but doesn't necessarily make sense to namespace. the solution in this PR is that because this is only a problem for node users anyhow, they don't have to use ChromeLauncher and we can leave chrome-launcher on the older version of lighthouse-logger :)

I see the pain point, but I also think we're heading in a direction where usage of LH as part of a larger application without worker barrier would be difficult for other reasons. A happy medium might be just the 'custom' logging level where we don't touch debug.enable

Related Issues/PRs
#5683

[wardpeet]

just to be sure, we are moving to relative require so we aren't coupled to the npm release cycle of lighthouse-logger?

[patrickhulce]

just to be sure, we are moving to relative require so we aren't coupled to the npm release cycle of lighthouse-logger?

well I did it in this PR so we could see how it feels without actually publishing :) if we decide to publish like this then we could remove the relative requires

[wardpeet]

ah wasn't sure about that we could test with yarn link lighthouse-logger so we don't have to change the require values.

PR title typo?
misc(logger): namescape debug with lh: => misc(logger): namespace debug with lh:

[patrickhulce]

we could test with yarn link lighthouse-logger so we don't have to change the require values.

ya that works too for now, I kinda meant that the resolution of ChromeLauncher issue might be not publishing this for awhile in which case the requires would need to be relative here until some later date that we move ChromeLauncher to something else (but at that point it'd be unnecessary to publish lighthouse-logger at all and we could leave them relative again :))

[wardpeet]

Ah I get it now! thanks!

[paulirish]

wfm

[brendankenny]

The lh prefixes are good, but this definitely makes the lighthouse-logger situation more confusing...it's an npm module in the lighthouse repo but lighthouse uses the local copy. It also means it goes back to being included twice in our bundle, as a module and as local required files.

It is lighthouse-logger, so I agree with the idea of ChromeLauncher just getting the lh prefixes unless anyone feels strongly about not getting them, at which point it should just be changed to use debug directly :)

[brendankenny]

feels like we shouldn't change this now? It's possible someone depends on this. We can make it available for those who want it, though

[patrickhulce]

This really is the most sane default for node module consumption though (which is the only channel it should affect). Just a TODO for next major then? 😢

[brendankenny]

You might be right, if you're using the node module and you care about the logging, you're probably most likely to have set quiet, but it just might be surprising why errors are suddenly not logged anymore?

[patrickhulce]

these modes are awful if you're actually using debug though because it disables all other modules output nevermind I forgot I just fixed that with scoping. I suppose we'll just add custom to the notes of the issue :)

[brendankenny]

the errors are because lighthouse-logger wasn't type checked, it was just using typings/lighthouse-logger since it was an external module

[patrickhulce]

If everyone's on board with ChromeLauncher being prefixed and reusing lighthouse-logger I'll remove the relative requires, publish and we're good to go 👍

[paulirish]

ya im on board with CL being prefixed

[patrickhulce]

Hm, I'm having trouble getting the failure case here to reproduce on master now.

const lighthouse = require('lighthouse')
const logA = require('debug')('logA')
const logB = require('debug')('logB')

(async () => {
  logA('hello')
  logB('hello')
  await lighthouse('https://example.com', {logLevel: 'silent'})
  logA('end')
  logB('end')
})()

the above does not seem to impact DEBUG=logA or DEBUG=logB at all :/

[brendankenny]

@patrickhulce should we close now or would you want to bring these changes along those discussed in #6313

[patrickhulce]

I don't think this is a real problem, I've been unable to get LH to mess with debug. Without clearer repro steps, I think we can close.