Skip to content
This repository has been archived by the owner on Jun 17, 2022. It is now read-only.

Use log service for console messages #221

Merged
merged 12 commits into from
Dec 11, 2020

Conversation

MGibson1
Copy link
Member

@MGibson1 MGibson1 commented Dec 4, 2020

This is a repeat of #214 with a shim layer for browser timing and added (any) tests.

Objective

Establish a centralized means of handling console logging. Partially resolves bitwarden/cli/issues/188, which calls out the need to shunt all stdout to stderr when the --response flag is used. This is the first step toward that.

Files changed

  • log.service.ts: node's console.time is used to measure time taken handling Ciphers and so must be implemented on logService.
  • cli/baseProgram.ts: --response flag is meant to guarantee json output to stdout.
  • consoleLogService.ts: Implement console.time similar to node's library. Output is in ms rather than s+ms.
  • electronLogService.ts: Same as consoleLogService.ts
  • baseImporter.ts: provide a basic consoleLogService in non-dev mode for importer. We may want to look at determining dev mode here. Use the logService for error reporting as we were previously using console.log/warn.
  • auth.service.ts/crypto.service.ts/notification.service.ts/search.service.ts: provide optional logService constructor parameter and default to consoleLogService in non-dev mode. We may want to look into determining dev mode. Use LogService for error reporting as we were previously using console.log/warn. The reason to use an optional parameter is because only CLI and Desktop use logService at the moment, by defaulting to a consoleLogService, we maintain behavior for web and browser without requiring code changes.

Changes from previous commit

Use browser-process-hrtime as a shim between node and browsers. This is well supported on canIUse.

I've also added tests of compatibility and behavior for base class, cli's log service, and electron log service. The base class is working in Karma and other tests are limited to Node.

Files

  • package-lock.json/package.json: add browser-process-hrtime and ts-node. ts-node is for local test running.
  • ** consoleLog.service.spec.ts**: test for base console log class. Tests browser shim and logging behavior.
  • electronLog.service.spec.ts: tests unique electron initializations.
  • cli/consoleLog.service.spec.ts: Tests --response override of log level and behavior of timer.
  • jasmine.json: include new test locations

This commit will require commensurate merges in /bitwarden/cli#197 and /bitwarden/desktop#602

@MGibson1 MGibson1 requested a review from a team December 4, 2020 23:15
@MGibson1 MGibson1 force-pushed the use-logService-for-console-messages branch from 70129e5 to 0bef569 Compare December 4, 2020 23:22
kspearrin
kspearrin previously approved these changes Dec 7, 2020
package.json Show resolved Hide resolved
src/services/search.service.ts Outdated Show resolved Hide resolved
src/services/search.service.ts Outdated Show resolved Hide resolved
src/services/notifications.service.ts Outdated Show resolved Hide resolved
src/services/crypto.service.ts Outdated Show resolved Hide resolved
src/services/auth.service.ts Outdated Show resolved Hide resolved
src/importers/baseImporter.ts Outdated Show resolved Hide resolved
src/importers/baseImporter.ts Show resolved Hide resolved
@MGibson1 MGibson1 force-pushed the use-logService-for-console-messages branch from edb8533 to 85c2745 Compare December 11, 2020 15:11
@MGibson1 MGibson1 merged commit 2c414ce into master Dec 11, 2020
@MGibson1 MGibson1 deleted the use-logService-for-console-messages branch December 11, 2020 16:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bw-cli sends unformatted data to stdout even when --response is used
3 participants