Skip to content

Conversation

@webJose
Copy link

@webJose webJose commented May 12, 2024

Motivation

I personally would like to remove minified error message 1 (see #1161), and others have expressed their desire to completely suppress logging.

With this solution, both are possible.

How It Works

A new utility module named logging has been added to the src/utils folder that exports 2 objects:

  1. The setLogger() function that controls the destination of all log entries.
  2. The proxy logger object, whose methods write to the log destination set by setLogger().

By default, the log destination is the console. Users can, at any point in their application's lifecycle, call setLogger() to control which log entries make it to the browser's console.

setLogger()

This function takes 1 argument that can be false, true or a custom destination log object.

The value false internally sets a log destination that discards all messages; the value true routes all messages to the console. The third option allows the caller to take complete control over what to do with the logged messages.

For Maintainers and Contributors

Never do console.debug(), console.info(), console.warn() or console.error() ever again. Always replace console with logger. Logger is imported:

import { logger } from 'path/to/src/utils/logging.js';

@webJose
Copy link
Author

webJose commented May 12, 2024

@MilanKovacic here's the drafted PR for the global logging feature. It is complete and contains 12 new unit tests. All the new tests are passing.

As seen in the PR, all previous unit testing related to console writing pass without modification, which should be a testament of the neutral nature of the change. This change is fully backwards compatible.

Please review and let me know if any changes are needed.

@MilanKovacic MilanKovacic self-requested a review May 15, 2024 22:28
Copy link

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Couple of small things. Thanks @webJose!

@webJose
Copy link
Author

webJose commented May 20, 2024

Changes applied!

@webJose
Copy link
Author

webJose commented May 22, 2024

Changes made. Additionally, I created the corresponding PR in the documentation website.

@ibacher
Copy link

ibacher commented May 24, 2024

@webJose Can you mark this as no longer a draft? @MilanKovacic what do you think about merging this?

@webJose webJose marked this pull request as ready for review May 24, 2024 14:40
@internettrans
Copy link
Member

internettrans commented Jun 8, 2024

This adds complexity and increases single-spa's API surface while providing little value. Logging to the console is ok. For comparison, ReactJS logs warnings to the console. Not calling start() within 5 seconds of the page being loaded is an indication of performance problems.

I'm closing this because I don't support the feature being added to single-spa

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

Successfully merging this pull request may close these issues.

3 participants