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

docs: update docs for logger, add API docs to website #7305

Merged
merged 2 commits into from
May 4, 2022
Merged

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented May 4, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

@docusaurus/logger is intended for public consumption (we even have a nice README for it), so let's add it to the website.

I don't know if it's worth figuring out how to deduplicate the two pages (README and the website), but considering the low update frequency of this package, I think it should be fine.

Test Plan

Test links

Deploy preview: https://deploy-preview-7305--docusaurus-2.netlify.app/docs/api/plugins/@docusaurus/logger/

@Josh-Cena Josh-Cena added the pr: documentation This PR works on the website or other text documents in the repo. label May 4, 2022
@Josh-Cena Josh-Cena requested review from slorber and lex111 as code owners May 4, 2022 09:34
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 4, 2022
@netlify
Copy link

netlify bot commented May 4, 2022

[V2]

Name Link
🔨 Latest commit cacf933
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62724a3cb6678100095e2a7b
😎 Deploy Preview https://deploy-preview-7305--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented May 4, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 63
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

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

@github-actions
Copy link

github-actions bot commented May 4, 2022

Size Change: +574 B (0%)

Total Size: 806 kB

Filename Size Change
website/.docusaurus/globalData.json 50.7 kB +166 B (0%)
website/build/assets/js/main.********.js 610 kB +408 B (0%)
ℹ️ View Unchanged
Filename Size
website/build/assets/css/styles.********.css 107 kB
website/build/index.html 38.8 kB

compressed-size-action

@Josh-Cena Josh-Cena merged commit 7a3894e into main May 4, 2022
@Josh-Cena Josh-Cena deleted the jc/logger-docs branch May 4, 2022 09:59
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Not against this, but please note that adding this to our doc means it now becomes public API surface, on which we'll want to maintain backward compatibility after 2.0

Make sure the public API suits you, because a breaking change later will be annoying for our plugin ecosystem (considering it's suggesting plugin authors to use it)

- `error`: prints an error (not necessarily halting the program) that signals significant problems.
- `success`: prints a success message.

:::caution A word on the `error` formatter
Copy link
Collaborator

Choose a reason for hiding this comment

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

not a big deal but these admonition titles might end up causing problems in Crowdin/i18n as translators end up translating the admonition key

@slorber
Copy link
Collaborator

slorber commented May 4, 2022

On my side what I want in the end is not to have one global logger for all plugins but rather one instance per plugin so that you have the ability to turn logging and verbosity level per plugin instance.

This might mean injecting the logger instance inside the plugin constructor

Not so sure encouraging plugin authors to use this package today is the right move, considering we may end up in a transition period where some plugins use the "local logger" while some use the statically imported global logger: ie you can only configure logging for some plugins, not all

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented May 4, 2022

It has already been public API and is being consumed by external plugins. You can see from the description that I do intend to let plugin authors use this.

one instance per plugin so that you have the ability to turn logging and verbosity level per plugin instance.

That seems weird... We'd rather only have one logging level globally, controlled by an env var.

Having a logger interface injected from the context makes it much, much harder to use, since if you split your functions, you have to pass it everywhere. That's the general problem of dependency injection, and I'd say let's not do it until there's a clear motivation to

@slorber
Copy link
Collaborator

slorber commented May 4, 2022

It has already been public API and is being consumed by external plugins. You can see from the description that I do intend to let plugin authors use this.

I understand that you do intend plugin authors to use it, once ready. The initial PR for me was meant to be used internally only.

Who is using this atm, and how did they discover the logging feature?


That seems weird... We'd rather only have one logging level globally, controlled by an env var.

That's not weird at all, this is a common logging practice across many languages

Java does it:

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class HelloWorld {
  public static void main(String[] args) {
    Logger logger = LoggerFactory.getLogger(HelloWorld.class);
    logger.info("Hello World");
  }
}

Many JS logging libs do it:

const winston = require(https://github.com/winstonjs/winston);

const logger = winston.createLogger({
  level: 'info',
});

Webpack does it: https://webpack.js.org/api/plugins/#logging

CleanShot 2022-05-04 at 16 34 41@2x


Having a logger interface injected from the context makes it much, much harder to use, since if you split your functions, you have to pass it everywhere. That's the general problem of dependency injection, and I'd say let's not do it until there's a clear motivation to

I understand that perfectly.

Exposing a createLogger(name) today and asking plugin authors to use might be a good enough compromise?

I'd really prefer if nobody (even ourselves) use that global logger singleton, and each log line should rather be associated to a given log context


In the Java/JVM world we can tune log levels on a class-by-class level, and all serious applications and third-party libs usually support that.

We don't need that granularity level but being able to turn on the verbose logs of a single plugin can be incredibly useful for debugging that plugin in particular, without having the logs polluted by verbosity of other plugins.

@Josh-Cena
Copy link
Collaborator Author

Who is using this atm, and how did they discover the logging feature?

You can check on the npm dependents graph. @RDIL has also told me he's using this internally. Like any mature project, utility packages should be designed with stability and reusability in mind, so being depended on by the community is a good thing.

About the createLogger, I absolutely agree. I actually want to do that as well, because I want to be able to add a unique message prefix for each plugin. I would send a PR very soon.

The idea of the current singleton is that it's stateless and just a bunch of utilities. I think that's a valuable behavior as well.

@Josh-Cena
Copy link
Collaborator Author

@slorber Feel free to mark this doc page as draft when you cut the release, if you don't want it to be in stable (yes... we can dogfood how draft works)

@slorber
Copy link
Collaborator

slorber commented May 4, 2022

Like any mature project, utility packages should be designed with stability and reusability in mind, so being depended on by the community is a good thing.

Mature projects add new public APIs sparingly because APIs are forever, particularly for infrastructure things like a logger which will be used by a plugin ecosystem.

because I want to be able to add a unique message prefix for each plugin

Yes, that's also a common practice to do so 👍


@slorber Feel free to mark this doc page as draft when you cut the release, if you don't want it to be in stable (yes... we can dogfood how draft works)

We can keep this, I suspect not a lot of users will start using this right now

and we would be able to migrate the default instance to something like

export default createLogger("other");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: documentation This PR works on the website or other text documents in the repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants