Skip to content

Direct all logging to console.warn instead of console.log#10234

Closed
cemerick wants to merge 1 commit intomozilla:masterfrom
cemerick:master
Closed

Direct all logging to console.warn instead of console.log#10234
cemerick wants to merge 1 commit intomozilla:masterfrom
cemerick:master

Conversation

@cemerick
Copy link
Contributor

@cemerick cemerick commented Nov 7, 2018

This avoids polluting stdout, important for node.js programs that are intended to produce machine-readable output.

(Ticket/PR suggested in IRC.)

This avoids polluting `stdout`, important for node.js programs that are
intended to produce machine-readable output.
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Nov 8, 2018

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/0eb1d98b93c0d64/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 8, 2018

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/0eb1d98b93c0d64/output.txt

Total script time: 3.01 mins

Published

function info(msg) {
if (verbosity >= VerbosityLevel.INFOS) {
console.log('Info: ' + msg);
console.warn('Info: ' + msg);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be console.info since information messages are now shown incorrectly as warnings in the console.

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 actually did that first, too, which is when I learned that, for reasons passing understanding, console.info is just an alias for console.log, and thus also pipes to stdout.

I think this means that there is truly no way for e.g. browser graphical consoles to usefully discriminate between simple log messages and warnings/errors, while also adhering to good stdout/stderr hygiene.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please note: It's easy to suppress all of these info/warn messages, if you simply include the verbosity parameter when calling getDocument. Following e.g. the pdf2svg example, you'd simply add verbosity: pdfjsLib.VerbosityLevel.ERRORS, to the parameters listed in

pdfjsLib.getDocument({
data: data,
// Try to export JPEG images directly if they don't need any further processing.
nativeImageDecoderSupport: pdfjsLib.NativeImageDecoding.DISPLAY
for all of these messages to be completely suppressed.

Would that be sufficient to work around the problem being solved here, since it seems a tiny bit unfortunate to have Node.js implementation details control exactly how the info/warn helper functions are implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I thought we were already setting verbosity (to warnings), but we actually weren't; it was being provided via the (maybe deprecated?) mechanism of setting options on a PDFJS global object prior to requiring/loading pdf.js. Moving that to the .getDocument() call does squelch the messages entirely.

Anyway, unstated in my initial problem description is that we do want to capture logs (ours as well as pdf.js'), in parallel with consuming the machine-readable output of the program; so, just squelching logging isn't great.

FWIW, including this in our program does accomplish our objectives:

console.log = console.info = console.warn;

We can simply carry on using that if you don't want to get wagged by the node.js tail here.

@timvandermeij
Copy link
Contributor

Closing since I also agree that it's better to leave this as-is as it's confusing to see information messages as warnings in the browser console and unfortunate to adapt the code specifically for a Node.js detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants