Replace `npmlog` with `consola` #829

br0p0p · 2024-07-01T19:03:05Z

This PR replaces the deprecated and dependency-laden npmlog with a smaller, modern alternative: consola.

	Module + Dependencies	Size on Disk (Unpacked Size)	Bundle size (minified)
`npmlog`	15	401 KB	65 KB
`consola`	1	222 KB	6 KB
𝛥	-14 (-93.3%)	-179 KB (−44.63%)	-59 KB (-90.7%)

This data was gathered from https://npmgraph.js.org/

Extrapolating based on the download stats for @mapbox/node-pre-gyp (5,730,435 downloads per week), this migration will have the following effects on the ecosystem as a whole:

80,226,090 fewer dependencies downloaded per week
Reduced network traffic by ~1 TB per week

package-size-calculator output

Package size report
===================

Package info for "@mapbox/[email protected]": 2.0 MB
  Released: 2023-07-14 13:09:45.532 +0000 UTC (52w6d ago)
  Downloads last week: 4,706,924 (80.46%)
  Estimated traffic last week: 9.3 TB
  Subdependencies: 57

Removed dependencies:
  - [email protected]: 401 kB (20.27%)
    Downloads last week: 7,502,133 (N/A% from 5.0.1)
    Downloads last week from "@mapbox/[email protected]": 4,706,924 (N/A%)
    Traffic last week: N/A
    Traffic from "@mapbox/[email protected]": 9.3 TB (N/A%)
    Subdependencies: 22 (38.59%)

Added dependencies:
  + [email protected]: 228 kB (14.47%)
    Downloads last week: 4,630,250 (N/A% from 3.2.3)
    Estimated traffic last week: N/A
    Subdependencies: 1 (1.75%)

Estimated package size: 2.0 MB → 1.8 MB (91.26%)
Estimated traffic over a week: 9.3 TB → 8.5 TB (813 GB saved)
Estimated traffic over a week @ 100% downloads: 12 TB → 11 TB (1.0 TB saved)
Subdependencies: 57 → 36 (-21)

I will note that the output looks slightly different with these changes. Happy to update if the new format is not great

Before

After

benmccann

Amazing! Thank you!!

benmccann · 2024-07-04T04:19:34Z

@br0p0p would you be able to fix the merge conflict?

br0p0p · 2024-07-05T01:46:04Z

@benmccann done

benmccann · 2024-07-05T02:09:37Z

lib/util/log.js

+// eslint-disable-next-line node/no-missing-require
+const { createConsola } = require('consola/basic');
+
+const log = createConsola({ stdout: process.stderr });


why put all output to stderr? that seems surprising. if we keep it we should at least add a comment to explain

I did this to maintain the default behavior of npmlog. See here and the README

Not exactly sure why this was the default

Probably a good call to add a comment at least. Maybe stderr makes sense here since the logger outputs more debugging type output and console.log() is used for user-facing output?

I think we should remove this rather than trying to maintain the behavior of npmlog. I checked (see below) and consola logs warnings and errors to stderr and info type logs to stdout, which seems like what I'd want and expect out of a logging system. Let's just check that we're setting the log levels appropriately whenever we do logging - I left one comment where I think we should do log.error to make sure an error goes to stdout

https://github.com/unjs/consola/blob/713ec3454889bf967e9c694bf800955f1cd60e52/src/reporters/basic.ts#L70
https://github.com/unjs/consola/blob/713ec3454889bf967e9c694bf800955f1cd60e52/src/constants.ts#L23

Sure thing. Updated.

My thinking was to prevent potentially breaking dependents which rely on the stderr output for whatever reason but I agree that not using stderr for everything makes sense

The main reason that most logging from npm and node-gyp goes to stderr (even now that neither use npmlog) is that stdout is reserved for output that is expected to be used programmatically (usually json). We don't usually swap which output stream is used based on flags like --json so the default is to place all logging on stderr so that if a json command is piped to something like jq the logging won't inadvertently create invalid json.

There are a lot of gray areas to this rule and this is something I spent a lot of time figuring out for npm in the past. I haven't actually review this code (yet) so I'm not saying that all output should be put on stderr, just that as a default stderr is usually better than stdout for CLI logging output.

Thanks for this context @lukekarrys. Thoughts @benmccann?

I have no objection to stderr given that additional context. If @lukekarrys thinks it's the way to go I'll defer to him on that

@br0p0p do you want to change this back to use stderr by default? It might be the safer option to avoid changing that

@benmccann sure, no problem

lib/install.js

benmccann

lgtm except that it looks like the appveyor tests might be failing

@cclauss could you kick off the github actions tests on this PR?

benmccann · 2024-07-08T14:12:09Z

@br0p0p I'm afraid all the CI jobs are failing with this PR. would you be able to take a look?

benmccann

Thank you @br0p0p !

@cclauss this one is now ready to go. It would be great to merge before the next preview release as it fixes a few deprecation warnings (I believe all the remaining).

HummingMind · 2024-11-11T01:20:57Z

Hoping this gets merged soon. Thank you for your work! 🍻

Replace npmlog with consola

a70be69

br0p0p marked this pull request as draft July 1, 2024 19:21

benmccann mentioned this pull request Jul 1, 2024

Deprecated subdependencies found in @mapbox/node-pre-gyp vercel/nft#421

Open

Rework printf style replacements

4c37295

br0p0p marked this pull request as ready for review July 2, 2024 03:08

benmccann approved these changes Jul 2, 2024

View reviewed changes

Merge branch 'master' into replace-npmlog

517049f

br0p0p requested a review from benmccann July 5, 2024 01:46

benmccann reviewed Jul 5, 2024

View reviewed changes

lib/install.js Outdated Show resolved Hide resolved

br0p0p added 2 commits July 6, 2024 22:31

Use log.error() instead of log.log()

f1db27e

Don't send everything to stderr

085d52b

benmccann approved these changes Jul 7, 2024

View reviewed changes

Fix test to check contents of stdout

f6a769d

benmccann approved these changes Jul 9, 2024

View reviewed changes

br0p0p added 2 commits July 16, 2024 11:27

Merge branch 'master' into replace-npmlog

a951f8e

Lockfile

4902b05

benmccann mentioned this pull request Jul 20, 2024

Needed: New maintainership of node-pre-gyp #657

Closed

ahmedelq mentioned this pull request Jul 21, 2024

Depreciation warnings for dependencies during installation #877

Open

br0p0p added 2 commits July 29, 2024 14:08

Merge branch 'master' into replace-npmlog

3ffc8c1

Merge branch 'mapbox:master' into replace-npmlog

db5366f

br0p0p requested a review from a team as a code owner September 10, 2024 21:41

Use stderr again

545d8e1

br0p0p requested review from benmccann and lukekarrys September 10, 2024 21:44

Update test

a51e1bb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `npmlog` with `consola` #829

Replace `npmlog` with `consola` #829

br0p0p commented Jul 1, 2024 •

edited

Loading

benmccann left a comment

benmccann commented Jul 4, 2024

br0p0p commented Jul 5, 2024

benmccann Jul 5, 2024

br0p0p Jul 5, 2024

benmccann Jul 5, 2024

br0p0p Jul 7, 2024 •

edited

Loading

lukekarrys Jul 19, 2024

br0p0p Jul 29, 2024

benmccann Jul 29, 2024

benmccann Sep 9, 2024

br0p0p Sep 10, 2024

benmccann left a comment

benmccann commented Jul 8, 2024

benmccann left a comment

HummingMind commented Nov 11, 2024

Replace npmlog with consola #829

Are you sure you want to change the base?

Replace npmlog with consola #829

Conversation

br0p0p commented Jul 1, 2024 • edited Loading

benmccann left a comment

Choose a reason for hiding this comment

benmccann commented Jul 4, 2024

br0p0p commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

br0p0p Jul 7, 2024 • edited Loading

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

benmccann left a comment

Choose a reason for hiding this comment

benmccann commented Jul 8, 2024

benmccann left a comment

Choose a reason for hiding this comment

HummingMind commented Nov 11, 2024

Replace `npmlog` with `consola` #829

Replace `npmlog` with `consola` #829

br0p0p commented Jul 1, 2024 •

edited

Loading

br0p0p Jul 7, 2024 •

edited

Loading