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

feature: extract lumberjack core into an agnostic library #154

Closed
wants to merge 99 commits into from

Conversation

NachoVazquez
Copy link
Contributor

@NachoVazquez NachoVazquez commented May 17, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: CONTRIBUTING.md#commit
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Lumberjack is a highly extensible and configurable logging library for Angular.

But why stop there?

What is the new behavior?

@webworker/lumberjack is a framework-agnostic logging library; it powers ngworker/lumberjack and enables the creation of any framework-specific lumberjack implementation like Qwik, Solid, or React.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Although nothing changes for @ngworker/lumberjack, this refactoring should introduce a major version.

@nx-cloud
Copy link

nx-cloud bot commented May 17, 2023

☁️ Nx Cloud Report

Attention: This version of the Nx Cloud GitHub bot will cease to function on July 1st, 2023. An organization admin can update your integration here.

CI is running/has finished running commands for commit 735764c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 7 targets

Sent with 💌 from NxCloud.

@NachoVazquez NachoVazquez added enhancement New feature or request lumberjack Applies to issues of the Lumberjack project. v16 This issue will be solve as part of the Lumberjack version 16 labels Jun 3, 2023
@NachoVazquez NachoVazquez force-pushed the feature/extracting-lumberjack-core branch 3 times, most recently from 49cf544 to 2279cb7 Compare June 9, 2023 12:46
@NachoVazquez NachoVazquez force-pushed the feature/extracting-lumberjack-core branch from 8d64cb4 to bfc6d1e Compare June 12, 2023 21:27
@NachoVazquez NachoVazquez force-pushed the main branch 4 times, most recently from 4c908ec to 9df215b Compare June 12, 2023 23:59
@NachoVazquez NachoVazquez force-pushed the feature/extracting-lumberjack-core branch from bfc6d1e to 2d38dd7 Compare June 13, 2023 20:03
@NachoVazquez NachoVazquez marked this pull request as ready for review June 13, 2023 22:28
@NachoVazquez NachoVazquez force-pushed the feature/extracting-lumberjack-core branch from 191c2bd to 2c7f384 Compare August 9, 2023 23:50
@nx-cloud
Copy link

nx-cloud bot commented Aug 9, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 91bf803. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 5 targets

Sent with 💌 from NxCloud.

Copy link
Member

@LayZeeDK LayZeeDK left a comment

Choose a reason for hiding this comment

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

Partial review of latest commits.

Comment on lines 68 to 70
return {
formatLog,
};
Copy link
Member

Choose a reason for hiding this comment

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

question: Does it makes sense to return an object? Can't we just return a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does make sense to return just a function. I was returning an object to keep it consistent with the other create* factory functions on the workspace.

If you think is ok to just return a function, I'm happy with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the two possibilities:

Returning an object

  const logFormatter = createLumberjackLogFormatter({ config, getUnixEpochTicks });
 
  const { log, formattedLog } = logFormatter.formatLog(lumberjackLog);

Returning a function

  const formatLog = createLumberjackLogFormatter({ config, getUnixEpochTicks });

  const { log, formattedLog } = formatLog(lumberjackLog);

@@ -24,7 +25,7 @@ import { LumberjackTimeService } from '../time/lumberjack-time.service';
* API.
*/
@Injectable()
export class LumberjackService<TPayload extends LumberjackLogPayload | void = void> {
export class LumberjackService<TPayload extends LumberjackLogPayload | void = void> implements Lumberjack<TPayload> {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can we please find a different name than LumberjackService? See #166 (comment).

Copy link
Contributor Author

@NachoVazquez NachoVazquez Aug 15, 2023

Choose a reason for hiding this comment

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

In my article about Plugin Architecture I use the terms core system and orchestrator.

Do any of those terms resonate with you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going with LumberjackOrchestrator. But we can change later.

constructor(
level: LumberjackLogLevel,
message: string,
getUnixEpochTicks: () => number = () => new Date().valueOf()
Copy link
Member

Choose a reason for hiding this comment

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

question: Do we have an integration test that replaces getUnixEpochTicks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In most tests, we use the fakeTime.getUnixEpochTicks if that's what you were thinking.

Copy link
Member

@LayZeeDK LayZeeDK left a comment

Choose a reason for hiding this comment

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

Request changes

- [@ngworker/lumberjack-applicationinsights-driver](https://github.com/ngworker/lumberjack-applicationinsights-driver), community log driver using [Azure Application Insights](https://docs.microsoft.com/en-us/azure/azure-monitor/app/app-insights-overview) as a log store.

The ngworkers teams offers hosting of a community log driver in the [ngworker GitHub](https://github.com/ngworker) and [@ngworker NPM](https://www.npmjs.com/org/ngworker) organizations.
The LumberjackJS team offers hosting of a community log driver in the [ngworker GitHub](https://github.com/ngworker) and [@ngworker NPM](https://www.npmjs.com/org/ngworker) organizations.
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: the Lumberjack team (JS is just for getting an npm scope).

Suggested change
The LumberjackJS team offers hosting of a community log driver in the [ngworker GitHub](https://github.com/ngworker) and [@ngworker NPM](https://www.npmjs.com/org/ngworker) organizations.
The Lumberjack team offers hosting of a community log driver in the [ngworker GitHub](https://github.com/ngworker) and [@ngworker NPM](https://www.npmjs.com/org/ngworker) organizations.

- [@ngworker/lumberjack-applicationinsights-driver](https://github.com/ngworker/lumberjack-applicationinsights-driver), community log driver using [Azure Application Insights](https://docs.microsoft.com/en-us/azure/azure-monitor/app/app-insights-overview) as a log store.

The ngworkers teams offers hosting of a community log driver in the [ngworker GitHub](https://github.com/ngworker) and [@ngworker NPM](https://www.npmjs.com/org/ngworker) organizations.
The LumberjackJS team offers hosting of a community log driver in the [ngworker GitHub](https://github.com/ngworker) and [@ngworker NPM](https://www.npmjs.com/org/ngworker) organizations.
Copy link
Member

Choose a reason for hiding this comment

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

issue: update GitHub and npm organizations

Suggested change
The LumberjackJS team offers hosting of a community log driver in the [ngworker GitHub](https://github.com/ngworker) and [@ngworker NPM](https://www.npmjs.com/org/ngworker) organizations.
The LumberjackJS team offers hosting of a community log driver in the [lumberjackjs GitHub](https://github.com/lumberjackjs) and [@lumberjackjs NPM](https://www.npmjs.com/org/lumberjackjs) organizations.

import { LumberjackConsoleDriverModule } from '@ngworker/lumberjack/console-driver';
import { LumberjackHttpDriverModule } from '@ngworker/lumberjack/http-driver';
import { LumberjackLevel, LumberjackModule } from '@lumberjackjs/angular';
import { LumberjackConsoleDriverModule } from '@lumberjackjs/angular/console-driver';
Copy link
Member

Choose a reason for hiding this comment

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

issue: our decision is to not have an Angular-specific console driver package or secondary entry point if possible, that is there will be a @lumberjackjs/core/console-driver package but no Angular console driver package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good catch, but I fear I must do a profound rewrite of the docs. Also, write down the migration guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I checked, and the docs are in bad shape. I will rewrite them next weekend.

import { LumberjackHttpDriverModule } from '@ngworker/lumberjack/http-driver';
import { LumberjackLevel, LumberjackModule } from '@lumberjackjs/angular';
import { LumberjackConsoleDriverModule } from '@lumberjackjs/angular/console-driver';
import { LumberjackHttpDriverModule } from '@lumberjackjs/angular/http-driver';
Copy link
Member

Choose a reason for hiding this comment

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

issue: correct the package name according to our decision

Suggested change
import { LumberjackHttpDriverModule } from '@lumberjackjs/angular/http-driver';
import { LumberjackHttpDriverModule } from '@lumberjackjs/angular-http-driver';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will rewrite the whole documentation next week.

import { provideLumberjackConsoleDriver } from '@ngworker/lumberjack/console-driver';
import { provideLumberjackHttpDriver, withHttpConfig } from '@ngworker/lumberjack/http-driver';
import { LumberjackLevel, provideLumberjack } from '@lumberjackjs/angular';
import { provideLumberjackConsoleDriver } from '@lumberjackjs/angular/console-driver';
Copy link
Member

Choose a reason for hiding this comment

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

issue: our decision is to not have an Angular-specific console driver package or secondary entry point if possible, that is there will be a @lumberjackjs/core/console-driver package but no Angular console driver package

@@ -1,6 +1,6 @@
import { Injectable } from '@angular/core';

import { LumberjackTimeService } from '@ngworker/lumberjack';
import { LumberjackTimeService } from '@lumberjackjs/angular';
Copy link
Member

Choose a reason for hiding this comment

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

question: where do we place LumberjackTimeService and will it have a different API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the agnostic core, I have replaced it with a getUnixEpochTicks function that can be passed to the createLumberjack factory function.

By default it uses the () => new Date().valueof()

This is the complete method signature

export function createLumberjack<TPayload extends LumberjackLogPayload | void = void>({
  drivers,
  config,
  getUnixEpochTicks = () => new Date().valueOf(),
}: LumberjackDependencies<TPayload>): Lumberjack<TPayload> 

packages/lumberjackjs/angular/console-driver/src/index.ts Outdated Show resolved Hide resolved
packages/lumberjackjs/angular/http-driver/src/index.ts Outdated Show resolved Hide resolved
@NachoVazquez NachoVazquez force-pushed the feature/extracting-lumberjack-core branch from fa79538 to 1bd120f Compare November 11, 2023 22:49
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

98.1% 98.1% Coverage
0.0% 0.0% Duplication

@LayZeeDK
Copy link
Member

LayZeeDK commented Jan 5, 2024

See #188

@LayZeeDK LayZeeDK closed this Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lumberjack Applies to issues of the Lumberjack project. v16 This issue will be solve as part of the Lumberjack version 16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants