refactor: unify log format with new logger utility #5994

Josh-Cena · 2021-11-23T09:22:54Z

Motivation

Continuing the spirit of #4579. We have seen a lot of libs migrating to picocolors because it's attractive in almost every way: small, performant, clean code... I see no reason to continue using chalk.

To prevent anyone from arguing over the ROI of this switch, we decided to encapsulate the coloring logic in a new package @docusaurus/logger. This package will be the only one doing the logging work. Read the README for that package:

`@docusaurus/logger`

An encapsulated logger for semantically formatting console messages.

APIs

It exports a single object as default export: logger. logger has the following properties:

All fields of picocolors. This includes yellow, createColors, isColorSupported, etc.
Formatters. These functions have the same signature as the formatters of picocolors. Note that their implementations are not guaranteed. You should only care about their semantics.
- path: formats a file path or URL.
- id: formats an identifier.
- code: formats a code snippet.
- subdue: subdues the text.
- num: formats a number.
The interpolate function. It is a template literal tag.
Logging functions. All logging functions can both be used as functions (in which it has the same usage as console.log) or template literal tags.
- info: prints information.
- warn: prints a warning that should be payed attention to.
- error: prints an error (not necessarily halting the program) that signals significant problems.
- success: prints a success message.

Using the template literal tag

The template literal tag evaluates the template and expressions embedded. interpolate returns a new string, while other logging functions prints it. Below is a typical usage:

logger.info`Hello %i${name}! You have %n${money} dollars. Here are the ${
  items.length > 1 ? 'items' : 'item'
} on the shelf: ${items}
To buy anything, enter %c${'buy x'} where %c${'x'} is the item's name; to quit, press %c${'Ctrl + C'}.`;

An embedded expression is optionally preceded by a flag in the form %[a-z]+ (a percentage sign followed by a few lowercase letters). If it's not preceded by any flag, it's printed out as-is. Otherwise, it's formatted with one of the formatters:

%p: path
%i: id
%c: code
%s: subdue
%n: num

If the expression is an array, it's formatted by `\n- ${array.join('\n- ')}\n` (note it automatically gets a leading line end). Each member is formatted by itself and the bullet is not formatted. So you would see the above message printed as:

[OMITTED]

netlify · 2021-11-23T09:30:10Z

✔️ [V2]

🔨 Explore the source changes: ac3f1af

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61c0a9d10e00f3000806dc65

😎 Browse the preview: https://deploy-preview-5994--docusaurus-2.netlify.app

github-actions · 2021-11-23T09:30:22Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	80
🟢 Accessibility	98
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	95

Lighthouse ran on https://deploy-preview-5994--docusaurus-2.netlify.app/

github-actions · 2021-11-23T09:41:56Z

Size Change: +450 B (0%)

Total Size: 652 kB

Filename	Size	Change
`website/build/assets/css/styles.********.css`	101 kB	+206 B (0%)
`website/build/index.html`	29.6 kB	+215 B (+1%)

ℹ️ View Unchanged

Filename	Size	Change
`website/.docusaurus/globalData.json`	37.8 kB	0 B
`website/build/assets/js/main.********.js`	484 kB	+29 B (0%)

_{compressed-size-action}

slorber · 2021-11-23T17:03:51Z

Honestly not really willing to do this change

You'll find a lot of context why tthere: babel/babel#13783 (comment)

It's not really worth it to change our logging lib every time there's a new one in town

However I'd like to abstract the underlying lib under our own abstraction and how logging levels for core, lifecycles and each plugins.

See some inspiration:

https://github.com/remotion-dev/remotion/blob/main/packages/cli/src/log.ts
Webpack logging system

armano2 · 2021-11-23T17:32:24Z

100% agree with @slorber, there is no reason to actually do swap,

creating abstract for logging levels is a great idea

but colors is just.... i feel like laughing

Josh-Cena · 2021-11-23T23:05:50Z

However I'd like to abstract the underlying lib under our own abstraction and how logging levels for core, lifecycles and each plugins.

Agree. So we create things like pathColor(), iddentifierColor(), infoColor() methods and import them instead of importing chalk, so that it doesn't matter what lib we use? I've thought about that

slorber · 2021-11-24T09:49:51Z

So we create things like pathColor(), iddentifierColor(), infoColor() methods

Not sure what you mean here.

I like the idea of having regular error levels like trace/info/warn/error (like the remotion example)
Do we need more coloring features outside of these use cases?

Josh-Cena · 2021-11-24T09:59:49Z

Do we need more coloring features outside of these use cases?

We use color-coding a lot. For example, the swizzling message uses almost all colors, and one success message includes green, cyan, and blue. These colors are also used quite inconsistently—although I don't think anyone would complain about cyan being used for a code identifier (Layout) in one place and a path (src/css/custom.css) in another, my OCD presses me to make a spec about color coding.

My idea is that each message can be classified as trace/info/warn/error as you mentioned, but also that within a message we may want to color-code certain entities, e.g. file paths, code identifiers. These can all be encapsulated and exposed as a declarative API so we can easily swap out the implementation.

slorber · 2021-11-24T10:48:00Z

👍 agree we should normalize these extra use-cases. I don't have a clear picture of all those use-cases though we can add them into a single place and iterate from there.

I'd like to have a docusaurus-logging package and only this one would depend on chalk, allowing to change the logging lib more seamlessly

Josh-Cena · 2021-11-24T10:53:52Z

I'd like to have a docusaurus-logging package and only this one would depend on chalk, allowing to change the logging lib more seamlessly

Whoa, that's more than I wished... I'm more thinking about @docusaurus/utils/consoleUtils. But anyways, we'll figure this out. I definitely think we should migrate to picocolors though, it's not just me who has a natural aversion to prototype modification (chalk.red.bold()). It's a pity that the fleet of Sindre Sorhus packages will always depend on chalk.

I'll not argue on this point though; I've observed many, many OSSes with multiple maintainers arguing over chalk vs. pico/nanocolors when I was doing the field research🤷‍♂️ The only ones that did the migration smoothly are those with a dictating maintainer.

Josh-Cena · 2021-11-28T09:05:26Z

People can try it out on their devices. Here are some demos.

slorber

The result looks good but I'm not sold on 2 things:

This PR is about creating the logging interface, not switching the underlying lib. We want it to be initially simple enough so that eventually switching lib later becomes easy, and could be done with a feature flag to remove the risk of that switch.
Do we really need template literals and interpolation?

slorber · 2021-12-01T10:09:13Z

packages/create-docusaurus/bin/index.js

-      ),
-  );
+  logger.error('Minimum Node.js version not met :(');
+  logger.info`You are using Node.js %n${process.version}, Requirement: Node.js %n${requiredVersion}.`;


even if it works, I don't really see the value of introducing a fancy logging syntax using template literals

never seen this anywhere before

slorber · 2021-12-01T10:11:15Z

packages/docusaurus-logger/package.json

+  },
+  "license": "MIT",
+  "dependencies": {
+    "picocolors": "^1.0.0"


I'd rather keep chalk really, changing this dependency that works has little value (unless you can measure a significant the perf impact?) and only introduce useless risk

i do agree with you, changing packages when current one is maintained its bad choice

chalk is not even removed from dependencies, as its used by underlying libs anyway

as for micro benchmarks, adding this interpolation is slower than adding 500 colors to message manually

It's rather about neither performance or bundle size, because as we realized, we neither get rid of Chalk nor save much perf with the template interpolation. But just as I said, picocolors is shinier because: (a) it doesn't pollute the prototype chain (I have such an aversion to Chalk's chalk.red.bold() magic) (b) it has a much smaller, cleaner API surface which allows us to re-export everything in an intuitive way (...pico) which just includes the functions we know we will need.

I'm fine with going back to Chalk. But as we are making a new package anyways, why don't we also choose a new lib that many big OSSes are already adopting?

the goal of this PR is not to change behavior and implementation, it's just to refactor to another logging architecture with less coupling leaking inside our codebase.

Please let's focus on what we decided to do: provide a logging abstraction that encapsulate the needs we have.

it has a much smaller, cleaner API surface which allows us to re-export everything in an intuitive way

the goal is 100% do actually not do that, that's the motivation behind this abstraction: not making the underlying leak into our codebase 😅

I'm fine with going back to Chalk. But as we are making a new package anyways, why don't we also choose a new lib that many big OSSes are already adopting?

That can be decided in a separate PR later, but be ready to have a lot of pushback from my side

In the meantime I prefer that we separate architecture/refactor changes from implementation details changes, and have many smaller PRs easier to review and agree on, compared to a larger PR that we disagree on many details

packages/docusaurus-theme-translations/package.json

packages/docusaurus-utils-validation/package.json

slorber · 2021-12-01T10:18:14Z

packages/docusaurus/bin/docusaurus.js

@@ -238,6 +239,7 @@ function isInternalCommand(command) {

 async function run() {
  if (!isInternalCommand(process.argv.slice(2)[0])) {
+    // @ts-expect-error: Hmmm


I'm enabling ts-check, but there's a type incompatibility. I don't have the bandwidth to investigate why that's the case for now, but I'll probably fix it before merge

slorber · 2021-12-01T10:28:01Z

packages/docusaurus-logger/src/index.ts

+  success,
+};
+
+export default logger;


Considering we'll want to provide configurable logging levels later, maybe exposing a static logger instance like this is not ideal? Not sure yet what our final API will look like though, we'll probably need more reactors later anyway

logger behaves like console in every way. I plan to make it read env vars when we support logging levels.

Later I'd like all loggers to have a name, they should rather be created and added to some context, eventually log their name to the console. We'd call something like createLogger()

Config could look like this:

const siteConfig = { logging: { core: "warn" browser: "error", plugins: { "docusaurus/plugin-content-docs": "info", "docusaurus/plugin-content-blog": "info", } } }

We can see how to implement that later though

slorber · 2021-12-01T10:28:32Z

packages/docusaurus-logger/src/index.ts

+}
+
+const logger = {
+  ...pico,


the point of this abstraction is actually to not expose the underlying lib to our own code, so pico/chakl should not be exposed

We occasionally need to use the imperative APIs (red, bold) as opposed to the semantic declarative ones (path, id). So the alternative to re-exposing pico is exporting every color, I'm fine with that as well

So the alternative to re-exposing pico is exporting every color, I'm fine with that as well

We should only expose colors that we actually use, it's like a "terminal design system / design tokens".

We really don't want to expose pico/chalk/whatever in any way here

packages/docusaurus-logger/src/index.ts

Josh-Cena · 2021-12-01T10:48:40Z

This PR is about creating the logging interface, not switching the underlying lib. We want it to be initially simple enough so that eventually switching lib later becomes easy, and could be done with a feature flag to remove the risk of that switch.

Picocolors and Chalk are exactly the same. I'm fine with moving it back to Chalk, but I'm unsure of why we don't want to move. This has absolutely no effect on the end user.

Do we really need template literals and interpolation?

The alternative of...

logger.warn`The following routes don't exist: %p${routes}`

...is this:

logger.warn(`The following routes don't exist:
- ${routes.map((route) => logger.path(route)).join('\n')});

You didn't see it nowhere. Chalk has templates which they removed in v5 because of the bundle size, but we can build our own version of it, tailored to our needs. Template tags allow us to transform arrays into the "list" form without writing out routes.map((route) => logger.path(route)).join('\n') every time, and also parse the string much more easily (compared to using regex to match every tag as in Chalk's {red {bold hey} josh}. The new templating approach they use is also tagged templates: https://github.com/chalk/chalk-template

I personally designed this API from the perspective of a Docusaurus developer. I've written many log messages scattered with things like ${chalk.cyan(path)} which I wish can be simplified. Also, this syntax prevents prettier from splitting the function call onto multiple links, which improves with readability actually. Considering logger.error(`Error while executing ${logger.code("docusaurus build")}`), I'm willing to change the template to logger.error`Error while executing %code${'docusaurus build'}` if that looks better?

slorber · 2021-12-03T19:10:49Z

Picocolors and Chalk are exactly the same. I'm fine with moving it back to Chalk, but I'm unsure of why we don't want to move. This has absolutely no effect on the end user.

If it's the same, then there's no reason to change.

If it's not the same, there's risk we'll take for no real benefits.

And this is not the initial goal of this PR

About the interpolation API, the best API is no API.

What you design here will become public API surface, as users will access their site plugins logger in site config.

I don't want to introduce a premature public API surface, nor to remove your interpolation API once this becomes exposed.

Let's do the simple thing first: just use regular JS logging features + some colors. No shorthands, no magic behavior for now

slorber

LGTM, let's move on and merge this for now

Josh-Cena added 2 commits November 23, 2021 17:13

refactor: migrate from chalk to picocolors

c1bad8d

Fix

4660e46

Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Nov 23, 2021

Josh-Cena requested review from lex111 and slorber as code owners November 23, 2021 09:22

facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 23, 2021

Josh-Cena added the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Nov 24, 2021

Josh-Cena added 3 commits November 28, 2021 08:44

Migrate some packages to logger

bc6645c

Merge branch 'main' into jc/picocolors

fe952e4

Fix package

77784f3

Josh-Cena changed the title ~~refactor: migrate from chalk to picocolors~~ refactor: unify log color-coding and formatting with new logger utility Nov 28, 2021

Josh-Cena marked this pull request as draft November 28, 2021 02:07

Josh-Cena added 6 commits November 28, 2021 10:12

Interpolate messages

b111968

Fix TS

f0c50ee

Fixes

2139b66

Better API

cfe7700

Fix

9ff817d

More migration

dd8f609

Josh-Cena removed the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Nov 28, 2021

Tweak

2508609

Fix snapshots

7b1cc11

Josh-Cena added 6 commits November 28, 2021 17:06

tweaks

9e2812c

Tweak

bb9bebc

Add tests

a0113a5

Fix test

c52228b

Fix

446aa2c

Tweaks

a7a99fc

slorber requested changes Dec 1, 2021

View reviewed changes

Josh-Cena added 6 commits December 3, 2021 21:09

More friendly syntax

e757074

id => name

8322a3d

Refactor theme-translations

fc4b0ea

Merge branch 'main' into jc/picocolors

0a706c0

Fix lock

577713c

Merge branch 'main' into jc/picocolors

689a5cd

Josh-Cena added 6 commits December 4, 2021 08:39

Hail chalk

5e54822

Fix

496245f

Merge branch 'main' into jc/picocolors

b484675

Fix test

326962a

Fix tests

55aded0

Fixes

47e25e7

Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Dec 19, 2021

remove picocolors deps

ac3f1af

slorber approved these changes Dec 20, 2021

View reviewed changes

slorber merged commit 770418f into main Dec 20, 2021

slorber deleted the jc/picocolors branch December 20, 2021 16:25

Josh-Cena removed the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Dec 20, 2021

mrazauskas mentioned this pull request Jun 26, 2022

test(logger): use jest-serializer-ansi-escapes #7685

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: unify log format with new logger utility #5994

refactor: unify log format with new logger utility #5994

Josh-Cena commented Nov 23, 2021 •

edited

Loading

netlify bot commented Nov 23, 2021 •

edited

Loading

github-actions bot commented Nov 23, 2021 •

edited

Loading

github-actions bot commented Nov 23, 2021 •

edited

Loading

slorber commented Nov 23, 2021

armano2 commented Nov 23, 2021 •

edited

Loading

Josh-Cena commented Nov 23, 2021

slorber commented Nov 24, 2021 •

edited

Loading

Josh-Cena commented Nov 24, 2021

slorber commented Nov 24, 2021

Josh-Cena commented Nov 24, 2021

Josh-Cena commented Nov 28, 2021

slorber left a comment

slorber Dec 1, 2021

slorber Dec 1, 2021

armano2 Dec 1, 2021

Josh-Cena Dec 1, 2021

slorber Dec 3, 2021

slorber Dec 1, 2021

Josh-Cena Dec 1, 2021

slorber Dec 1, 2021

Josh-Cena Dec 1, 2021

slorber Dec 3, 2021

slorber Dec 1, 2021

Josh-Cena Dec 1, 2021

slorber Dec 3, 2021

Josh-Cena commented Dec 1, 2021 •

edited

Loading

slorber commented Dec 3, 2021

slorber left a comment

refactor: unify log format with new logger utility #5994

refactor: unify log format with new logger utility #5994

Conversation

Josh-Cena commented Nov 23, 2021 • edited Loading

Motivation

@docusaurus/logger

APIs

Using the template literal tag

netlify bot commented Nov 23, 2021 • edited Loading

github-actions bot commented Nov 23, 2021 • edited Loading

github-actions bot commented Nov 23, 2021 • edited Loading

slorber commented Nov 23, 2021

armano2 commented Nov 23, 2021 • edited Loading

Josh-Cena commented Nov 23, 2021

slorber commented Nov 24, 2021 • edited Loading

Josh-Cena commented Nov 24, 2021

slorber commented Nov 24, 2021

Josh-Cena commented Nov 24, 2021

Josh-Cena commented Nov 28, 2021

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josh-Cena commented Dec 1, 2021 • edited Loading

slorber commented Dec 3, 2021

slorber left a comment

Choose a reason for hiding this comment

Josh-Cena commented Nov 23, 2021 •

edited

Loading

`@docusaurus/logger`

netlify bot commented Nov 23, 2021 •

edited

Loading

github-actions bot commented Nov 23, 2021 •

edited

Loading

github-actions bot commented Nov 23, 2021 •

edited

Loading

armano2 commented Nov 23, 2021 •

edited

Loading

slorber commented Nov 24, 2021 •

edited

Loading

Josh-Cena commented Dec 1, 2021 •

edited

Loading