lockbox-extension

lockbox-extension copied to clipboard

When should we use console.log() and friends? Should we still have eslint warn whenever we use it? And so on...

I believe our policy recently has been no console logging. Shall we just draw the line there and say that's our policy for now?

I see 3 console.log in the code base today (1 commented out):

https://github.com/mozilla-lockbox/lockbox-extension/search?utf8=%E2%9C%93&q=console.log&type=

Given the above, shall we remove those entirely?

Looks like we currently have 8 instances of console.{error,log,warn} (7 of which are ignored using eslint-disable-next-line):

$ git grep "eslint-disable-next-line no-console" | wc -l # 7

$ git grep -B1 "console\."

src/bootstrap.js-      // eslint-disable-next-line no-console
src/bootstrap.js:      console.log("telemetry events already registered; skipping registration");
--
src/webextension/background/accounts/index.js-    const oauthInfo = await fetchFromEndPoint("token", url, request);
src/webextension/background/accounts/index.js:    // console.log(`oauth info == ${JSON.stringify(oauthInfo)}`);
--
src/webextension/background/accounts/index.js-    // eslint-disable-next-line no-console
src/webextension/background/accounts/index.js:    console.log(`loaded account for (${account.mode.toString()}) '${account.uid || ""}'`);
--
src/webextension/background/accounts/index.js-    // eslint-disable-next-line no-console
src/webextension/background/accounts/index.js:    console.error(`loading account failed (fallback to empty GUEST): ${err.message}`);
--
src/webextension/background/views.js-      // eslint-disable-next-line no-console
src/webextension/background/views.js:      console.error(`could not close ${this.path} view: ${err.message}`);
--
src/webextension/list/manage/telemetry.js-    // eslint-disable-next-line no-console
src/webextension/list/manage/telemetry.js:    console.error("Unable to record telemetry event", e);
--
src/webextension/list/popup/telemetry.js-    // eslint-disable-next-line no-console
src/webextension/list/popup/telemetry.js:    console.error("Unable to record telemetry event", e);
--
test/unit/mocks/browser.js-      // eslint-disable-next-line no-console
test/unit/mocks/browser.js:      console.warn("Warning: only one listener supported; did you forget to " +

@devinreams removing all is not appropriate. We need a couple of these in src as checkpoints; the rest if something does unexpected but not outright failed.

I think the one in test is worth keeping because it points out a difference in API behavior between "real" listeners and this mock.

I actually think it'd be good to move towards more logging, not less. Especially during alpha, it'd be really useful for us to be able to say "just give us a debug log" when something goes wrong, instead of having to go find them in person and walk through the steps.

We could probably strip out unwanted logging during Webpack bundling using something like webpack-strip. That would allow us to easily retain debugging for local builds, but disable when we build for production builds. I think we could even configure to remove console.log() but keep a potentially more serious console.warn() and console.error(), if we wanted.

Conversely, not sure how difficult it'd be to let users toggle verbose console logging on production builds via a checkbox in about:addons config page.

What are the downsides to a lot of console logging? As long as the logging we do is intentional, and not just random junk from development, I think we're fine.