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.

Sign up for GitHub

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

Jump to bottom

chore: unify logger #3121

Merged
merged 2 commits into from
Oct 28, 2024
Merged

chore: unify logger #3121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9397cd9
chore: unified logger
2heal1 Oct 28, 2024
0025e85
fix: external rslog
2heal1 Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .changeset/fast-apples-double.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
---
'@module-federation/bridge-shared': patch
'@module-federation/bridge-react': patch
'@module-federation/bridge-vue3': patch
'@module-federation/rsbuild-plugin': patch
'@module-federation/data-prefetch': patch
'@module-federation/retry-plugin': patch
'@module-federation/dts-plugin': patch
'@module-federation/enhanced': patch
'@module-federation/managers': patch
'@module-federation/manifest': patch
'@module-federation/modern-js': patch
'@module-federation/runtime': patch
'@module-federation/sdk': patch
---

chore: unified logger
1 change: 1 addition & 0 deletions packages/bridge/bridge-react/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"dependencies": {
"@loadable/component": "^5.16.4",
"@module-federation/bridge-shared": "workspace:*",
"@module-federation/sdk": "workspace:*",
"react-error-boundary": "^4.0.13"
},
"peerDependencies": {
Expand Down
6 changes: 4 additions & 2 deletions packages/bridge/bridge-react/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import React from 'react';
import { Logger } from '@module-federation/bridge-shared';
import { createLogger } from '@module-federation/sdk';

export const LoggerInstance = new Logger('bridge-react');
export const LoggerInstance = createLogger(
'[ Module Federation Bridge React ]',
);
Comment on lines +4 to +6
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ]',
));


type typeReact = typeof React;
Copy link
Contributor

Choose a reason for hiding this comment

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

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;


Expand Down
1 change: 0 additions & 1 deletion packages/bridge/bridge-shared/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
export type { RenderFnParams, ProviderParams } from './type';
export { Logger } from './logger';
export { dispatchPopstateEnv } from './env';
52 changes: 0 additions & 52 deletions packages/bridge/bridge-shared/src/logger.ts
View file
Open in desktop

This file was deleted.

3 changes: 2 additions & 1 deletion packages/bridge/vue3-bridge/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
"vue-router": "=3"
},
"dependencies": {
"@module-federation/bridge-shared": "workspace:*"
"@module-federation/bridge-shared": "workspace:*",
"@module-federation/sdk": "workspace:*"
},
"devDependencies": {
"@vitejs/plugin-vue": "^5.0.4",
Expand Down
4 changes: 2 additions & 2 deletions packages/bridge/vue3-bridge/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
import { Logger } from '@module-federation/bridge-shared';
import { createLogger } from '@module-federation/sdk';

export const LoggerInstance = new Logger('vue3-bridge');
export const LoggerInstance = createLogger('[ Module Federation Bridge Vue3 ]');
6 changes: 4 additions & 2 deletions packages/data-prefetch/src/logger/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { Logger } from '@module-federation/sdk';
import { createLogger } from '@module-federation/sdk';

export default new Logger('[Module Federation Data Prefetch]');
const logger = createLogger('[ Module Federation Data Prefetch ]');

export default logger;
10 changes: 4 additions & 6 deletions packages/dts-plugin/src/core/lib/DTSManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
REMOTE_ALIAS_IDENTIFIER,
HOST_API_TYPES_FILE_NAME,
} from '../constant';
import { fileLog } from '../../server';
import { fileLog, logger } from '../../server';
import { axiosGet, cloneDeepOptions, isDebugMode } from './utils';
import { UpdateMode } from '../../server/constant';

Expand Down Expand Up @@ -163,12 +163,10 @@ class DTSManager {
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;
Comment on lines 163 to 171
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
}

}
Expand Down Expand Up @@ -406,7 +404,7 @@ class DTSManager {
this.consumeAPITypes(hostOptions);
}

console.log(ansiColors.green('Federated types extraction completed'));
logger.success('Federated types extraction completed');
} catch (err) {
if (this.options.host?.abortOnError === false) {
fileLog(
Comment on lines +407 to 410
Copy link
Contributor

Choose a reason for hiding this comment

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

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}`);

Expand Down
21 changes: 11 additions & 10 deletions packages/dts-plugin/src/plugins/DevPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
WEB_CLIENT_OPTIONS_IDENTIFIER,
WebClientOptions,
getIPV4,
logger,
} from '../server';
import type { Compiler, WebpackPluginInstance } from 'webpack';
import path from 'path';
Expand Down Expand Up @@ -56,32 +57,32 @@ export class DevPlugin implements WebpackPluginInstance {

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);
});
Comment on lines 58 to 71
Copy link
Contributor

Choose a reason for hiding this comment

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

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'));
}

}

private _handleUnexpectedExit(): void {
process.on('unhandledRejection', (error) => {
console.error('Unhandled Rejection Error: ', error);
console.log(
chalk`{cyan ${this._options.name} Process(${process.pid}) unhandledRejection, mf server will exit...}`,
logger.error(error);
logger.error(
`Process(${process.pid}) unhandledRejection, mf server will exit...`,
);
this._exit(PROCESS_EXIT_CODE.FAILURE);
});
process.on('uncaughtException', (error) => {
console.error('Unhandled Rejection Error: ', error);
console.log(
chalk`{cyan ${this._options.name} Process(${process.pid}) uncaughtException, mf server will exit...}`,
logger.error(error);
logger.error(
`Process(${process.pid}) uncaughtException, mf server will exit...`,
);
this._exit(PROCESS_EXIT_CODE.FAILURE);
});
Comment on lines 74 to 88
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the signal handlers, the error handling code has duplicate patterns. Consider consolidating the error handlers to improve maintainability:

Suggested change
private _handleUnexpectedExit(): void {
process.on('unhandledRejection', (error) => {
console.error('Unhandled Rejection Error: ', error);
console.log(
chalk`{cyan ${this._options.name} Process(${process.pid}) unhandledRejection, mf server will exit...}`,
logger.error(error);
logger.error(
`Process(${process.pid}) unhandledRejection, mf server will exit...`,
);
this._exit(PROCESS_EXIT_CODE.FAILURE);
});
process.on('uncaughtException', (error) => {
console.error('Unhandled Rejection Error: ', error);
console.log(
chalk`{cyan ${this._options.name} Process(${process.pid}) uncaughtException, mf server will exit...}`,
logger.error(error);
logger.error(
`Process(${process.pid}) uncaughtException, mf server will exit...`,
);
this._exit(PROCESS_EXIT_CODE.FAILURE);
});
private _handleUnexpectedExit(): void {
const handleError = (errorType: string, error: Error) => {
logger.error(error);
logger.error(
`Process(${process.pid}) ${errorType}, mf server will exit...`
);
this._exit(PROCESS_EXIT_CODE.FAILURE);
};
process.on('unhandledRejection', (error) =>
handleError('unhandledRejection', error as Error)
);
process.on('uncaughtException', (error) =>
handleError('uncaughtException', error)
);
}

Expand Down
5 changes: 1 addition & 4 deletions packages/dts-plugin/src/server/WebClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
import WebSocket from 'isomorphic-ws';
import {
DEFAULT_WEB_SOCKET_PORT,
WEB_SOCKET_CONNECT_MAGIC_ID,
} from './constant';
import { DEFAULT_WEB_SOCKET_PORT } from './constant';
import { Message } from './message/Message';
import { AddWebClientAction } from './message/Action';
import { APIKind, ReloadWebClientAPI } from './message/API';
Expand Down
8 changes: 5 additions & 3 deletions packages/dts-plugin/src/server/utils/log.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { logger } from '@module-federation/sdk';
import { createLogger } from '@module-federation/sdk';
import * as log4js from 'log4js';
import chalk from 'chalk';
Copy link
Contributor

Choose a reason for hiding this comment

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

The log4js import appears to be unused. If it's not needed elsewhere in the file, consider removing it to avoid unused dependencies.

import { MF_SERVER_IDENTIFIER } from '../constant';
Copy link
Contributor

Choose a reason for hiding this comment

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

The chalk import appears to be unused. If it's not needed elsewhere in the file, consider removing it to avoid unused dependencies.

import { ActionKind } from '../message/Action';

const logger = createLogger(`[ ${MF_SERVER_IDENTIFIER} ]`);

function log(msg: string): void {
logger.info(chalk`{bold {greenBright [ ${MF_SERVER_IDENTIFIER} ]} ${msg}}`);
logger.info(msg);
}

Comment on lines 9 to 11
Copy link
Contributor

Choose a reason for hiding this comment

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

The log function is a simple wrapper that doesn't add value beyond the logger.info call. Consider either:

  1. Removing it and using logger.info directly
  2. Adding additional functionality to justify its existence
  3. If kept, add JSDoc to explain its purpose

Here's a suggestion with JSDoc:

Suggested change
function log(msg: string): void {
logger.info(chalk`{bold {greenBright [ ${MF_SERVER_IDENTIFIER} ]} ${msg}}`);
logger.info(msg);
}
/**
* Logs an informational message with the Module Federation server identifier
* @param msg The message to log
*/
function log(msg: string): void {
logger.info(msg);
}

function fileLog(msg: string, module: string, level: string) {
Expand Down Expand Up @@ -35,4 +37,4 @@ function error(error: unknown, action: ActionKind, from: string): string {
return err.toString();
}

export { log, fileLog, error };
export { log, fileLog, error, logger };
3 changes: 2 additions & 1 deletion packages/enhanced/src/lib/container/ModuleFederationPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { StatsPlugin } from '@module-federation/manifest';
import {
composeKeyWithSeparator,
type moduleFederationPlugin,
logger,
} from '@module-federation/sdk';
import { PrefetchPlugin } from '@module-federation/data-prefetch/cli';
import { normalizeWebpackPath } from '@module-federation/sdk/normalize-webpack-path';
Expand Down Expand Up @@ -110,7 +111,7 @@ class ModuleFederationPlugin implements WebpackPluginInstance {
if (err instanceof Error) {
err.message = `[ ModuleFederationPlugin ]: Manifest will not generate, because: ${err.message}`;
}
console.warn(err);
logger.warn(err);
Comment on lines 111 to +114
Copy link
Contributor

Choose a reason for hiding this comment

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

The error handling could be improved by providing more context and using a more appropriate logging level for this scenario. Since this is a configuration issue that affects the plugin's core functionality, it should be logged as an error instead of a warning. Here's a better approach:

Suggested change
if (err instanceof Error) {
err.message = `[ ModuleFederationPlugin ]: Manifest will not generate, because: ${err.message}`;
}
console.warn(err);
logger.warn(err);
if (err instanceof Error) {
err.message = `[ ModuleFederationPlugin ]: Manifest will not generate, because: ${err.message}`;
logger.error(err);
} else {
logger.error('[ ModuleFederationPlugin ]: Manifest generation failed with an unknown error');
}

This change:

  1. Uses error level logging for better visibility of this important issue
  2. Handles non-Error objects explicitly
  3. Maintains consistent error message formatting

disableManifest = true;
}
}
Expand Down
11 changes: 6 additions & 5 deletions packages/enhanced/src/lib/container/runtime/ChildCompilationRuntimePlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import fs from 'fs';
import path from 'path';
import { ConcatSource } from 'webpack-sources';
import { transformSync } from '@swc/core';
import { logger } from '@module-federation/sdk';

const { RuntimeModule, Template, RuntimeGlobals } = require(
normalizeWebpackPath('webpack'),
Comment on lines 8 to 14
Copy link
Contributor

Choose a reason for hiding this comment

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

The code uses require with normalizeWebpackPath for webpack imports while using ES module imports for other dependencies. For consistency and better maintainability, consider using ES module imports for webpack dependencies as well:

Suggested change
import path from 'path';
import { ConcatSource } from 'webpack-sources';
import { transformSync } from '@swc/core';
import { logger } from '@module-federation/sdk';
const { RuntimeModule, Template, RuntimeGlobals } = require(
normalizeWebpackPath('webpack'),
import path from 'path';
import { ConcatSource } from 'webpack-sources';
import { transformSync } from '@swc/core';
import { logger } from '@module-federation/sdk';
import { RuntimeModule, Template, RuntimeGlobals } from 'webpack';

Expand Down Expand Up @@ -142,7 +143,7 @@ class CustomRuntimePlugin {
childCompiler.options.devtool = undefined;
childCompiler.options.optimization.splitChunks = false;
childCompiler.options.optimization.removeAvailableModules = true;
console.log('Creating child compiler for', this.bundlerRuntimePath);
logger.log('Creating child compiler for', this.bundlerRuntimePath);
Comment on lines 143 to +146
Copy link
Contributor

Choose a reason for hiding this comment

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

The child compiler configuration is setting critical options without documenting why these specific settings are necessary. Add a comment explaining the rationale behind disabling devtool and splitChunks, and enabling removeAvailableModules:

Suggested change
childCompiler.options.devtool = undefined;
childCompiler.options.optimization.splitChunks = false;
childCompiler.options.optimization.removeAvailableModules = true;
console.log('Creating child compiler for', this.bundlerRuntimePath);
logger.log('Creating child compiler for', this.bundlerRuntimePath);
// Disable development tools and chunk splitting for the child compiler
// to ensure clean, optimized output without source maps or code splitting
childCompiler.options.devtool = undefined;
childCompiler.options.optimization.splitChunks = false;
childCompiler.options.optimization.removeAvailableModules = true;
logger.log('Creating child compiler for', this.bundlerRuntimePath);


childCompiler.hooks.thisCompilation.tap(
this.constructor.name,
Expand Down Expand Up @@ -174,7 +175,7 @@ class CustomRuntimePlugin {
onceForCompilationMap.set(compilation, source);
onceForCompilationMap.set(compiler, source);
fs.writeFileSync(outputPath, source);
console.log('got compilation asset');
logger.log('got compilation asset');
childCompilation.chunks.forEach((chunk) => {
chunk.files.forEach((file) => {
childCompilation.deleteAsset(file);
Comment on lines 175 to 181
Copy link
Contributor

Choose a reason for hiding this comment

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

The code writes to the filesystem synchronously which could block the event loop. Consider using asynchronous file operations. Also, improve error handling for the file write operation:

Suggested change
onceForCompilationMap.set(compilation, source);
onceForCompilationMap.set(compiler, source);
fs.writeFileSync(outputPath, source);
console.log('got compilation asset');
logger.log('got compilation asset');
childCompilation.chunks.forEach((chunk) => {
chunk.files.forEach((file) => {
childCompilation.deleteAsset(file);
onceForCompilationMap.set(compilation, source);
onceForCompilationMap.set(compiler, source);
try {
fs.writeFileSync(outputPath, source);
logger.log('got compilation asset');
childCompilation.chunks.forEach((chunk) => {
chunk.files.forEach((file) => {
childCompilation.deleteAsset(

Expand All @@ -195,7 +196,7 @@ class CustomRuntimePlugin {
}

if (!childCompilation) {
console.warn(
logger.warn(
'Embed Federation Runtime: Child compilation is undefined',
);
return callback();
Expand All @@ -205,7 +206,7 @@ class CustomRuntimePlugin {
return callback(childCompilation.errors[0]);
}

console.log('Code built successfully');
logger.log('Code built successfully');

callback();
Comment on lines +209 to 211
Copy link
Contributor

Choose a reason for hiding this comment

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

The success log message lacks context about what code was built. Add more specific information to aid in debugging:

Suggested change
logger.log('Code built successfully');
callback();
logger.log(`Child compilation code built successfully for ${this.bundlerRuntimePath}`);
callback();

},
Expand Down Expand Up @@ -233,7 +234,7 @@ class CustomRuntimePlugin {
);

compilation.addRuntimeModule(chunk, runtimeModule);
console.log(`Custom runtime module added to chunk: ${chunk.name}`);
logger.log(`Custom runtime module added to chunk: ${chunk.name}`);
Comment on lines 236 to +237
Copy link
Contributor

Choose a reason for hiding this comment

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

The log message could be more informative by including additional context about the runtime module being added:

Suggested change
compilation.addRuntimeModule(chunk, runtimeModule);
console.log(`Custom runtime module added to chunk: ${chunk.name}`);
logger.log(`Custom runtime module added to chunk: ${chunk.name}`);
compilation.addRuntimeModule(chunk, runtimeModule);
logger.log(`Added custom runtime module '${runtimeModule.constructor.name}' to chunk: ${chunk.name || chunk.id}`);

};
compilation.hooks.runtimeRequirementInTree
.for(federationGlobal)
Expand Down
4 changes: 2 additions & 2 deletions packages/managers/src/PKGJsonManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import path from 'path';
import finder from 'find-pkg';
import fs from 'fs';
import { MFModuleType } from '@module-federation/sdk';
import { MFModuleType, logger } from '@module-federation/sdk';

export class PKGJsonManager {
private _pkg?: Record<string, any>;
Comment on lines 6 to 7
Copy link
Contributor

Choose a reason for hiding this comment

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

The type annotation for _pkg has a typo in 'string' which would cause a compilation error. The correct type should be:

Suggested change
export class PKGJsonManager {
private _pkg?: Record<string, any>;
export class PKGJsonManager {
private _pkg?: Record<string, any>;

Expand All @@ -27,7 +27,7 @@ export class PKGJsonManager {
this._pkg = pkg;
return pkg;
} catch (err) {
console.error(err);
logger.error(err);
return {};
Comment on lines 29 to 31
Copy link
Contributor

Choose a reason for hiding this comment

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

The error handling here silently fails and returns an empty object, which could mask serious issues. Consider adding error details to the log message and potentially throwing a more specific error. Here's a suggested improvement:

Suggested change
} catch (err) {
console.error(err);
logger.error(err);
return {};
} catch (err) {
logger.error('Failed to read package.json:', err);
throw new Error(`Unable to parse package.json: ${err.message}`);
}

}
}
Expand Down
14 changes: 7 additions & 7 deletions packages/manifest/src/ManifestManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,14 @@ import {
Stats,
Manifest,
ManifestExpose,
StatsExpose,
StatsShared,
ManifestShared,
ManifestRemote,
StatsRemote,
moduleFederationPlugin,
encodeName,
MFPrefetchCommon,
} from '@module-federation/sdk';
import { getFileName, isDev } from './utils';
import logger from './logger';
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using a more specific logger import to avoid potential naming conflicts and improve code clarity. Import the logger with a more descriptive name that indicates its purpose: ```suggestion
import manifestLogger from './logger';


For the second code block (lines 143-154):

import type { Compilation, Compiler } from 'webpack';
import { PLUGIN_IDENTIFIER } from './constants';
import { ManifestInfo } from './types';
Expand Down Expand Up @@ -145,10 +143,12 @@ class ManifestManager {
}

if (isDev()) {
console.log(
chalk`{bold {greenBright [ ${PLUGIN_IDENTIFIER} ]} {greenBright Manifest Link:} {cyan ${
publicPath === 'auto' ? '{auto}/' : publicPath
}${manifestFileName}}}`,
logger.log(
`Manifest Link: ${chalk.cyan(
`${
publicPath === 'auto' ? '{auto}/' : publicPath
}${manifestFileName}`,
)} `,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

The log message could be improved for better readability and debugging. Consider using template literals and adding more context about what this manifest link represents: ```suggestion
logger.log(
Manifest Link (${process.env.NODE_ENV}): ${chalk.cyan( ${publicPath === 'auto' ? '{auto}/' : publicPath}${manifestFileName}`
)}`
);

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extracting the manifest link generation logic into a separate function to improve code organization and reusability. This would also make the code more testable:

Suggested change
if (isDev()) {
console.log(
chalk`{bold {greenBright [ ${PLUGIN_IDENTIFIER} ]} {greenBright Manifest Link:} {cyan ${
publicPath === 'auto' ? '{auto}/' : publicPath
}${manifestFileName}}}`,
logger.log(
`Manifest Link: ${chalk.cyan(
`${
publicPath === 'auto' ? '{auto}/' : publicPath
}${manifestFileName}`,
)} `,
);
}
const getManifestLink = (publicPath: string, manifestFileName: string) => {
const basePath = publicPath === 'auto' ? '{auto}/' : publicPath;
return `${basePath}${manifestFileName}`;
};
if (isDev()) {
logger.log(`Manifest Link: ${chalk.cyan(getManifestLink(publicPath, manifestFileName))}`);
}


Expand Down
10 changes: 5 additions & 5 deletions packages/manifest/src/StatsManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
/* eslint-disable @typescript-eslint/member-ordering */
/* eslint-disable max-depth */

import chalk from 'chalk';
import {
StatsRemote,
StatsBuildInfo,
Expand Down Expand Up @@ -31,6 +30,7 @@ import {
getFileName,
getTypesMetaInfo,
} from './utils';
import logger from './logger';
import {
ContainerManager,
RemoteManager,
Expand Down Expand Up @@ -490,13 +490,13 @@ class StatsManager {
} = compiler.options;

if (typeof publicPath !== 'string') {
console.warn(
chalk`{bold {yellow [ ${PLUGIN_IDENTIFIER} ]: Manifest will not generate, because publicPath can only be string, but got '${publicPath}' }}`,
logger.warn(
`Manifest will not generate, because publicPath can only be string, but got '${publicPath}'`,
);
Comment on lines +493 to 495
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using a more descriptive error message that explains what valid values are expected for publicPath. This helps developers understand how to fix the issue:

Suggested change
logger.warn(
`Manifest will not generate, because publicPath can only be string, but got '${publicPath}'`,
);
`Manifest generation failed: publicPath must be a string. Received type: '${typeof publicPath}', value: '${publicPath}'`,

return false;
} else if (publicPath === 'auto') {
console.warn(
chalk`{bold {blue [ ${PLUGIN_IDENTIFIER} ]: Manifest will use absolute path resolution via its host at runtime, reason: publicPath='${publicPath}'}}`,
logger.warn(
`Manifest will use absolute path resolution via its host at runtime, reason: publicPath='${publicPath}'`,
);
return true;
}
Comment on lines 492 to 502
Copy link
Contributor

Choose a reason for hiding this comment

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

The error handling for publicPath validation could be improved by consolidating the checks and making the logic more maintainable. Here's a suggested refactoring:

Suggested change
if (typeof publicPath !== 'string') {
console.warn(
chalk`{bold {yellow [ ${PLUGIN_IDENTIFIER} ]: Manifest will not generate, because publicPath can only be string, but got '${publicPath}' }}`,
logger.warn(
`Manifest will not generate, because publicPath can only be string, but got '${publicPath}'`,
);
return false;
} else if (publicPath === 'auto') {
console.warn(
chalk`{bold {blue [ ${PLUGIN_IDENTIFIER} ]: Manifest will use absolute path resolution via its host at runtime, reason: publicPath='${publicPath}'}}`,
logger.warn(
`Manifest will use absolute path resolution via its host at runtime, reason: publicPath='${publicPath}'`,
);
return true;
}
if (typeof publicPath !== 'string') {
logger.warn(
`Manifest will not generate, because publicPath can only be string, but got '${publicPath}'`,
);
return false;
}
if (publicPath === 'auto') {
logger.warn(
'Manifest will use absolute path resolution via its host at runtime, reason: publicPath="auto"',
);
return true;
}

This improves the code by:

  1. Using early return pattern for cleaner flow
  2. Avoiding else-if chain for better readability
  3. Using consistent string quotes in the warning message
  4. Making the 'auto' message more explicit by hardcoding the value instead of string interpolation

Expand Down
Loading
Loading