chore: unify logger #3121
Merged
chore: unify logger #3121
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 0025e85 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for module-federation-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![squadronai[bot]](https://avatars.githubusercontent.com/in/901096?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
The pull request unifies the logging implementation across the project by replacing the custom Logger class with the isomorphic-rslog library. This simplifies the logging setup and provides a consistent logging experience throughout the application. The key changes include:
- Replacing the use of
console.log,console.error, and custom logging utilities with theisomorphic-rsloglibrary across various packages and modules. - Introducing a centralized logger with a consistent prefix to improve the structure and context of log messages.
- Leveraging the capabilities of
isomorphic-rslogto support different log levels, including debug mode. - Streamlining imports and removing unused dependencies related to logging to improve code organization and maintainability.
These changes aim to unify the logging mechanism and provide a more robust, standardized, and easily manageable logging solution for the entire codebase.
File Summaries
| File | Summary |
|---|---|
| packages/bridge/bridge-react/src/utils.ts | The code changes unify the logging implementation across the project by replacing the custom Logger class with the createLogger function from the @module-federation/sdk package. This simplifies the logging setup and provides a consistent logging experience throughout the application. |
| packages/bridge/bridge-shared/src/index.ts | The code changes unify the logging implementation by replacing the custom Logger class with the isomorphic-rslog library. This simplifies the codebase and provides a consistent logging solution across the application. |
| packages/bridge/vue3-bridge/src/utils.ts | The code changes unify the logging mechanism across the project by replacing the custom Logger class with the createLogger function from the @module-federation/sdk package. This change simplifies the logging implementation and ensures a consistent logging experience throughout the application. |
| packages/dts-plugin/src/core/lib/DTSManager.ts | The code changes unify the logging mechanism by replacing the use of console.log and console.error with a centralized logging utility called logger. This includes replacing the usage of ansiColors.green and ansiColors.red with the corresponding logger.success and logger.error methods. The primary purpose of these changes is to provide a consistent and standardized logging approach throughout the codebase. |
| packages/dts-plugin/src/plugins/DevPlugin.ts | The code changes unify the logging mechanism by replacing the use of console.log and console.error with a centralized logging library, isomorphic-rslog. This improves the consistency and maintainability of the logging system across the application. |
| packages/dts-plugin/src/server/WebClient.ts | The code changes introduce a simplification by removing unnecessary imports related to the WEB_SOCKET_CONNECT_MAGIC_ID constant, as it is no longer used in the updated code. The primary purpose of these changes is to streamline the imports and remove unused dependencies, improving the overall code organization and maintainability. |
| packages/dts-plugin/src/server/utils/log.ts | The code changes introduce a new logger implementation using the isomorphic-rslog library to unify the logging across the application. The primary changes include replacing the existing logger import with a new createLogger function, which creates a logger instance with a custom prefix. Additionally, the log function has been simplified to use the new logger instance directly, removing the custom formatting. |
| packages/enhanced/src/lib/container/ModuleFederationPlugin.ts | The code changes introduce the use of the isomorphic-rslog library to unify the logging functionality across the codebase. The primary purpose of this change is to provide a consistent and standardized logging mechanism, which can be used throughout the application. |
| packages/enhanced/src/lib/container/runtime/ChildCompilationRuntimePlugin.ts | The code changes introduce the use of the isomorphic-rslog library to unify the logging functionality across the application. The changes replace the existing console.log and console.warn statements with calls to the logger object from the @module-federation/sdk package, which is likely a wrapper around the isomorphic-rslog library. This change ensures consistent logging behavior throughout the codebase. |
| packages/managers/src/PKGJsonManager.ts | The code changes introduce the use of the isomorphic-rslog library to unify the logging functionality across the codebase. The primary modification is the replacement of console.error() calls with logger.error() from the @module-federation/sdk package, which now utilizes the isomorphic-rslog library for consistent logging behavior. |
| packages/manifest/src/ManifestManager.ts | The code changes introduce the use of the isomorphic-rslog library to unify the logging functionality across the codebase. The primary purpose of this change is to provide a consistent and standardized logging solution that can be used in both the client and server-side environments. |
| packages/manifest/src/StatsManager.ts | The code changes unify the logging mechanism by replacing the use of the chalk library with a custom logger implementation from the isomorphic-rslog package. This change ensures consistent logging across the application, making it easier to manage and maintain the logging functionality. |
| packages/manifest/src/StatsPlugin.ts | The code changes introduce a new logger utility to centralize logging functionality across the project. This helps unify the logging approach and provides a consistent way to handle errors and log messages throughout the codebase. |
| packages/manifest/src/logger.ts | This change unifies the logging implementation across the codebase by introducing a centralized logger using the isomorphic-rslog library. The new logger is configured with a custom prefix to identify the specific plugin context, providing a more structured and consistent logging experience throughout the application. |
| packages/manifest/src/utils.ts | The code changes unify the logging mechanism by replacing the use of the chalk library with a custom logger implementation from the ./logger module. This change simplifies the logging process and ensures a consistent logging experience throughout the codebase. |
| packages/runtime/src/module/index.ts | The code changes introduce a new logger utility to replace the use of console.error in the Module class. This change aims to unify the logging mechanism across the codebase by using the isomorphic-rslog library, which provides a consistent logging interface that can be used in both client-side and server-side environments. |
| packages/runtime/src/remote/index.ts | The changes in this file introduce a new logger utility to replace the use of console.log for logging purposes. The primary purpose of this change is to unify the logging mechanism across the codebase, likely to provide more consistent and structured logging capabilities. |
| packages/runtime/src/utils/logger.ts | The code changes introduce a new logging utility that unifies the logging across the application. The primary purpose is to use the isomorphic-rslog library to create a centralized logger with a consistent log category prefix. The changes also include updates to the existing assert, error, and warn functions to leverage the new logger. |
| packages/sdk/src/env.ts | The code changes introduce a new function isBrowserDebug() that checks if the browser's local storage has a specific key-value pair, and uses this information to determine if the application is in debug mode. Additionally, the isDebugMode() function has been updated to prioritize the browser's debug mode over the FEDERATION_DEBUG environment variable. |
| packages/sdk/src/logger.ts | The code changes unify the logging functionality across the application by replacing the custom logger implementation with the isomorphic-rslog library. The new logger provides a consistent interface and supports different log levels, including debug mode. The changes also introduce a prefix for the log messages to provide better context. |
| packages/sdk/src/utils.ts | The code changes introduce a unified logging mechanism using the isomorphic-rslog library. The primary purpose is to centralize the logging functionality and provide a consistent logging experience across the application. The changes involve replacing the custom Logger implementation with the logger instance from the isomorphic-rslog library, ensuring a more robust and standardized logging solution. |
There was a problem hiding this comment.
Incremental Review
Comments posted: 38
Configuration
Squadron Mode: essential
Commits Reviewed
837da819b612813d2886ad030a1d3c0a986ec3ed...9397cd9089bce4cb0b4ea61a9d69fc57ccfb842f
Files Reviewed
- packages/bridge/bridge-react/src/utils.ts
- packages/bridge/vue3-bridge/src/utils.ts
- packages/dts-plugin/src/core/lib/DTSManager.ts
- packages/dts-plugin/src/plugins/DevPlugin.ts
- packages/dts-plugin/src/server/WebClient.ts
- packages/dts-plugin/src/server/utils/log.ts
- packages/enhanced/src/lib/container/ModuleFederationPlugin.ts
- packages/enhanced/src/lib/container/runtime/ChildCompilationRuntimePlugin.ts
- packages/managers/src/PKGJsonManager.ts
- packages/manifest/src/ManifestManager.ts
- packages/manifest/src/StatsManager.ts
- packages/manifest/src/StatsPlugin.ts
- packages/manifest/src/utils.ts
- packages/runtime/src/module/index.ts
- packages/runtime/src/remote/index.ts
- packages/runtime/src/utils/logger.ts
- packages/sdk/src/env.ts
- packages/sdk/src/logger.ts
- packages/sdk/src/utils.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/fast-apples-double.md
- packages/bridge/bridge-react/package.json
- packages/bridge/vue3-bridge/package.json
- packages/data-prefetch/src/logger/index.ts
- packages/modernjs/src/cli/logger.ts
- packages/modernjs/src/cli/ssrPlugin.ts
- packages/modernjs/src/cli/utils.ts
- packages/retry-plugin/package.json
- packages/retry-plugin/src/constant.ts
- packages/retry-plugin/src/fetch-retry.ts
- packages/retry-plugin/src/logger.ts
- packages/retry-plugin/src/script-retry.ts
- packages/rsbuild-plugin/src/cli/index.ts
- packages/rsbuild-plugin/src/logger.ts
- packages/sdk/package.json
- pnpm-lock.yaml
Comment on lines
+4
to
+6
| export const LoggerInstance = createLogger( | ||
| '[ Module Federation Bridge React ]', | ||
| ); |
There was a problem hiding this comment.
The logger instance is exported as a mutable variable. Consider making it readonly to prevent accidental modifications:
Suggested change
| export const LoggerInstance = createLogger( | |
| '[ Module Federation Bridge React ]', | |
| ); | |
| export const LoggerInstance = Object.freeze(createLogger( | |
| '[ Module Federation Bridge React ]', | |
| )); |
| export const LoggerInstance = new Logger('bridge-react'); | ||
| export const LoggerInstance = createLogger( | ||
| '[ Module Federation Bridge React ]', | ||
| ); | ||
|
|
||
| type typeReact = typeof React; |
There was a problem hiding this comment.
The type alias 'typeReact' uses a non-standard naming convention. Consider using PascalCase for type definitions:
Suggested change
| type typeReact = typeof React; | |
| type TypeReact = typeof React; |
Comment on lines
163
to
171
| console.error(err); | ||
| } | ||
| } | ||
| console.log(ansiColors.green('Federated types created correctly')); | ||
| logger.success('Federated types created correctly'); | ||
| } catch (error) { | ||
| if (this.options.remote?.abortOnError === false) { | ||
| console.error( | ||
| ansiColors.red(`Unable to compile federated types, ${error}`), | ||
| ); | ||
| logger.error(`Unable to compile federated types, ${error}`); | ||
| } else { | ||
| throw error; |
There was a problem hiding this comment.
The error handling here could be improved by providing more detailed error information. Instead of directly passing the error object to the logger, extract useful information and format it better:
Suggested change
| console.error(err); | |
| } | |
| } | |
| console.log(ansiColors.green('Federated types created correctly')); | |
| logger.success('Federated types created correctly'); | |
| } catch (error) { | |
| if (this.options.remote?.abortOnError === false) { | |
| console.error( | |
| ansiColors.red(`Unable to compile federated types, ${error}`), | |
| ); | |
| logger.error(`Unable to compile federated types, ${error}`); | |
| } else { | |
| throw error; | |
| if (this.options.remote?.abortOnError === false) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| const errorStack = error instanceof Error ? error.stack : undefined; | |
| logger.error(`Unable to compile federated types: ${errorMessage}${errorStack ? `\nStack: ${errorStack}` : ''}`); | |
| } else { | |
| throw error; | |
| } |
Comment on lines
+407
to
410
| logger.success('Federated types extraction completed'); | ||
| } catch (err) { | ||
| if (this.options.host?.abortOnError === false) { | ||
| fileLog( |
There was a problem hiding this comment.
The error handling at line 409-410 is inconsistent with the pattern used elsewhere in the code. It switches from using the logger to using fileLog directly. For consistency and better error reporting:
Suggested change
| logger.success('Federated types extraction completed'); | |
| } catch (err) { | |
| if (this.options.host?.abortOnError === false) { | |
| fileLog( | |
| logger.success('Federated types extraction completed'); | |
| } catch (err) { | |
| if (this.options.host?.abortOnError === false) { | |
| logger.error(`Unable to consume federated types: ${err}`); |
Comment on lines
58
to
71
| private _stopWhenSIGTERMOrSIGINT(): void { | ||
| process.on('SIGTERM', () => { | ||
| console.log( | ||
| chalk`{cyan ${this._options.name} Process(${process.pid}) SIGTERM, mf server will exit...}`, | ||
| logger.info( | ||
| `${this._options.name} Process(${process.pid}) SIGTERM, mf server will exit...`, | ||
| ); | ||
| this._exit(PROCESS_EXIT_CODE.SUCCESS); | ||
| }); | ||
|
|
||
| process.on('SIGINT', () => { | ||
| console.log( | ||
| chalk`{cyan ${this._options.name} Process(${process.pid}) SIGINT, mf server will exit...}`, | ||
| logger.info( | ||
| `${this._options.name} Process(${process.pid}) SIGINT, mf server will exit...`, | ||
| ); | ||
| this._exit(PROCESS_EXIT_CODE.SUCCESS); | ||
| }); |
There was a problem hiding this comment.
The signal handling code could be refactored to reduce duplication and improve maintainability. Consider consolidating the SIGTERM and SIGINT handlers since they perform identical actions:
Suggested change
| private _stopWhenSIGTERMOrSIGINT(): void { | |
| process.on('SIGTERM', () => { | |
| console.log( | |
| chalk`{cyan ${this._options.name} Process(${process.pid}) SIGTERM, mf server will exit...}`, | |
| logger.info( | |
| `${this._options.name} Process(${process.pid}) SIGTERM, mf server will exit...`, | |
| ); | |
| this._exit(PROCESS_EXIT_CODE.SUCCESS); | |
| }); | |
| process.on('SIGINT', () => { | |
| console.log( | |
| chalk`{cyan ${this._options.name} Process(${process.pid}) SIGINT, mf server will exit...}`, | |
| logger.info( | |
| `${this._options.name} Process(${process.pid}) SIGINT, mf server will exit...`, | |
| ); | |
| this._exit(PROCESS_EXIT_CODE.SUCCESS); | |
| }); | |
| private _stopWhenSIGTERMOrSIGINT(): void { | |
| const handleSignal = (signal: string) => { | |
| logger.info( | |
| `${this._options.name} Process(${process.pid}) ${signal}, mf server will exit...` | |
| ); | |
| this._exit(PROCESS_EXIT_CODE.SUCCESS); | |
| }; | |
| process.on('SIGTERM', () => handleSignal('SIGTERM')); | |
| process.on('SIGINT', () => handleSignal('SIGINT')); | |
| } |
Comment on lines
+31
to
+33
| if (typeof FEDERATION_DEBUG !== 'undefined' && Boolean(FEDERATION_DEBUG)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The condition can be simplified by combining the type check and boolean conversion into a single expression:
Suggested change
| if (typeof FEDERATION_DEBUG !== 'undefined' && Boolean(FEDERATION_DEBUG)) { | |
| return true; | |
| } | |
| if (FEDERATION_DEBUG != null) { | |
| return true; | |
| } |
This is more concise while maintaining the same behavior, since we only care if the value exists rather than its specific boolean value.
Comment on lines
+23
to
+33
| function createLogger(prefix: string) { | ||
| const loggerInstance = _createLogger({ | ||
| labels: { | ||
| warn: `${PREFIX} Warn`, | ||
| error: `${PREFIX} Error`, | ||
| success: `${PREFIX} Success`, | ||
| info: `${PREFIX} Info`, | ||
| ready: `${PREFIX} Ready`, | ||
| debug: `${PREFIX} Debug`, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The createLogger function has redundant label initialization. The initial labels with PREFIX are immediately overwritten by setPrefix. Remove the initial labels object:
Suggested change
| function createLogger(prefix: string) { | |
| const loggerInstance = _createLogger({ | |
| labels: { | |
| warn: `${PREFIX} Warn`, | |
| error: `${PREFIX} Error`, | |
| success: `${PREFIX} Success`, | |
| info: `${PREFIX} Info`, | |
| ready: `${PREFIX} Ready`, | |
| debug: `${PREFIX} Debug`, | |
| }, | |
| }); | |
| function createLogger(prefix: string) { | |
| const loggerInstance = _createLogger(); | |
| setDebug(loggerInstance); | |
| setPrefix(loggerInstance, prefix); | |
| return loggerInstance; | |
| } |
Comment on lines
+12
to
21
| function setPrefix(loggerInstance: Logger, prefix: string) { | ||
| loggerInstance.labels = { | ||
| warn: `${prefix} Warn`, | ||
| error: `${prefix} Error`, | ||
| success: `${prefix} Success`, | ||
| info: `${prefix} Info`, | ||
| ready: `${prefix} Ready`, | ||
| debug: `${prefix} Debug`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The setPrefix function could be simplified by using an array of label types to reduce repetition and make maintenance easier:
Suggested change
| function setPrefix(loggerInstance: Logger, prefix: string) { | |
| loggerInstance.labels = { | |
| warn: `${prefix} Warn`, | |
| error: `${prefix} Error`, | |
| success: `${prefix} Success`, | |
| info: `${prefix} Info`, | |
| ready: `${prefix} Ready`, | |
| debug: `${prefix} Debug`, | |
| }; | |
| } | |
| function setPrefix(loggerInstance: Logger, prefix: string) { | |
| const labelTypes = ['warn', 'error', 'success', 'info', 'ready', 'debug']; | |
| loggerInstance.labels = Object.fromEntries( | |
| labelTypes.map(type => [type, `${prefix} ${type.charAt(0).toUpperCase() + type.slice(1)}`]) | |
| ); | |
| } |
|
|
||
| const DEBUG_LOG = '[ FEDERATION DEBUG ]'; | ||
| const PREFIX = '[ Module Federation ]'; |
There was a problem hiding this comment.
Consider making the prefix string a constant object to avoid potential typos and enable better IDE support:
Suggested change
| const PREFIX = '[ Module Federation ]'; | |
| const PREFIXES = { | |
| MODULE_FEDERATION: '[ Module Federation ]' | |
| } as const; |
Comment on lines
+6
to
10
| function setDebug(loggerInstance: Logger) { | ||
| if (isDebugMode()) { | ||
| loggerInstance.level = 'verbose'; | ||
| } | ||
| } |
There was a problem hiding this comment.
The setDebug function could benefit from a more descriptive name and a return value for better function composition:
Suggested change
| function setDebug(loggerInstance: Logger) { | |
| if (isDebugMode()) { | |
| loggerInstance.level = 'verbose'; | |
| } | |
| } | |
| function configureDebugLevel(loggerInstance: Logger): Logger { | |
| if (isDebugMode()) { | |
| loggerInstance.level = 'verbose'; | |
| } | |
| return loggerInstance; | |
| } |
There was a problem hiding this comment.
Incremental Review
Comments posted: 2
Configuration
Squadron Mode: essential
Commits Reviewed
9397cd9089bce4cb0b4ea61a9d69fc57ccfb842f...0025e8599fd41e6ddc57ccecc01d8b869a5e4cb2
Files Reviewed
- packages/manifest/src/ManifestManager.ts
- packages/manifest/src/logger.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/fast-apples-double.md
- packages/bridge/bridge-react/package.json
- packages/bridge/vue3-bridge/package.json
- packages/data-prefetch/src/logger/index.ts
- packages/modernjs/src/cli/logger.ts
- packages/modernjs/src/cli/ssrPlugin.ts
- packages/modernjs/src/cli/utils.ts
- packages/retry-plugin/package.json
- packages/retry-plugin/src/constant.ts
- packages/retry-plugin/src/fetch-retry.ts
- packages/retry-plugin/src/logger.ts
- packages/retry-plugin/src/script-retry.ts
- packages/rsbuild-plugin/src/cli/index.ts
- packages/rsbuild-plugin/src/logger.ts
- packages/sdk/package.json
- packages/sdk/rollup.config.js
- pnpm-lock.yaml
| chalk`{bold {greenBright [ ${PLUGIN_IDENTIFIER} ]} {greenBright Manifest Link:} {cyan ${ | ||
| publicPath === 'auto' ? '{auto}/' : publicPath | ||
| }${manifestFileName}}}`, | ||
| logger.info( |
There was a problem hiding this comment.
The log level is set to info which might be too verbose for development environment checks. For development-only logs, consider using debug level instead: ```suggestion
logger.debug(
Comment on lines
145
to
+149
| if (isDev()) { | ||
| console.log( | ||
| chalk`{bold {greenBright [ ${PLUGIN_IDENTIFIER} ]} {greenBright Manifest Link:} {cyan ${ | ||
| publicPath === 'auto' ? '{auto}/' : publicPath | ||
| }${manifestFileName}}}`, | ||
| logger.info( | ||
| `Manifest Link: ${chalk.cyan( | ||
| `${ | ||
| publicPath === 'auto' ? '{auto}/' : publicPath |
There was a problem hiding this comment.
The code appears to be incomplete with a truncated variable p. This could lead to runtime errors. Ensure the string template literal is properly closed and all variables are fully referenced: ```suggestion
if (isDev()) {
logger.debug(
Manifest Link: ${chalk.cyan( publicPath )}
);
}
zhoushaw
approved these changes
Oct 28, 2024
Merged
|
Looks like adding isomorphic-rslog to repo break something . When we try to run our micro-frontend with "@module-federation/enhanced": "0.6.14" we get this error. it is working as expected with NPM but not with PNPM. pnpm -v 9.12.3 |
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
use isomorphic-rslog to unify log
Related Issue
Types of changes
Checklist