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

Don't log messages to the console that the user or developer can do nothing about #11292

Closed
KeithHenry opened this issue Oct 31, 2019 · 2 comments

Comments

@KeithHenry
Copy link

We're developing an app that uses pdf.js - we use the browser console extensively during development but don't log anything to it (unless absolutely necessary) in production. We deal with millions of PDFs from different sources, often converted poorly or corrupted before we get them.

pdf.js is excellent, but logs warnings and errors to the console that the developer or end user can do nothing about.

For instance #3768 - a bad font recovery message. That's useful information, but not to the end user viewing the PDF or the application developer.

Other warnings are useful but with an unhelpful and persistent message, for instance #10377 if you await page.render(... you get told that then syntax is deprecated. The fix (to await page.render(...).promise) and related documentation to it should be indicated, and if the deprecated method is still called in production then end users don't need to know about it.

What is the expected behaviour?

In the production build:

  • Issues handled internally should not be logged to the console.
  • Information about using the API can be logged to the console but
    • should be written for an external developer with links to documentation.
    • can be turned off in production environments or alternate development contexts.
  • Issues that some end users in some contexts could act on (for instant a bad font message telling a PDF author to change the PDF) need to be some kind of event, observer, subscription or callback, as there's no way to promote the console log messages to actual UI.
@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Oct 31, 2019

[...] but logs warnings and errors to the console that the developer or end user can do nothing about.

I suppose that it depends on what sort of "developer" we're talking about here, since I can assure you that from the PDF.js-contributor perspective many of the console messages are most helpful.
As for the end-user part, console messages are definitely not useless there either since they often allow users to provide additional and useful information when reporting bugs.

[...] and if the deprecated method is still called in production then end users don't need to know about it.

It's, generally speaking, the responsibility of the person actually utilizing the PDF.js library to ensure that their code is using correct and current APIs, and otherwise update their own code such that deprecated messages aren't seen by end-users.
In particular, you probably don't want to deploy a new PDF.js release without testing it yourself first.

Issues handled internally should not be logged to the console.

The amount of logging printed in the console can be easily controlled by the API user when calling getDocument, please see

pdf.js/src/display/api.js

Lines 152 to 153 in 72bd8e8

* @property {number} [verbosity] - Controls the logging level; the
* constants from {VerbosityLevel} should be used.
and the currently available values in

pdf.js/src/shared/util.js

Lines 182 to 186 in 72bd8e8

const VerbosityLevel = {
ERRORS: 0,
WARNINGS: 1,
INFOS: 5,
};

For example, calling getDocument in the following way will only print actual error messages:

const loadingTask = getDocument({
  url,
  verbosity: 0,
});

Information about using the API can be logged to the console but

should be written for an external developer with links to documentation.

This is an open source project, with most people contributing in their spare time. Please understand that writing good documentation can be both time-consuming and non-trivial, so if the documentation is lacking please consider contributing to it :-)

can be turned off in production environments or alternate development contexts.

It's not clear what you mean by "production environments" here, but if you're e.g. referring to the pre-built pdf.js/pdf.worker.js files that's distributed here and here then those need to have API warnings printed by default since otherwise the implementer wouldn't get notified about e.g. deprecated APIs.

[...] need to be some kind of event, observer, subscription or callback, as there's no way to promote the console log messages to actual UI.

There already is such a thing[1], for unsupported features, please see

pdf.js/src/display/api.js

Lines 463 to 467 in 72bd8e8

/**
* Callback to when unsupported feature is used. The callback receives
* an {UNSUPPORTED_FEATURES} argument.
*/
this.onUnsupportedFeature = null;

For an example of how this can be used, please see the default viewer code here and here.


[1] This is e.g. being used to display a notification bar, for features which aren't supported, in the built-in Firefox PDF Viewer.

@timvandermeij
Copy link
Contributor

Closing as answered; I also agree with everything said above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants