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

plugin use infrastructureLogger and output the report.html to compiler.outputFileSystem #477

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

plugin use infrastructureLogger and output the report.html to compiler.outputFileSystem #477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e343d8a
fix: plugin uses infrastructureLogger and outputs the report.html to …
wood1986 Oct 13, 2021
8183d69
fix: generateStatsFile should use utils.mkdirSync
wood1986 Oct 13, 2021
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: 12 additions & 5 deletions src/BundleAnalyzerPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
const fs = require('fs');
const path = require('path');
const {bold} = require('chalk');

const Logger = require('./Logger');
const viewer = require('./viewer');
const utils = require('./utils');
const {writeStats} = require('./statsUtils');
const PLUGIN_NAME = 'webpack-bundle-analyzer';

class BundleAnalyzerPlugin {
constructor(opts = {}) {
Expand Down Expand Up @@ -35,6 +35,8 @@ class BundleAnalyzerPlugin {
this.compiler = compiler;

const done = (stats, callback) => {
const isWebpack5 = !!compiler.webpack;
this.fs = isWebpack5 ? compiler.outputFileSystem : require('fs');
callback = callback || (() => {});

const actions = [];
Expand Down Expand Up @@ -72,15 +74,18 @@ class BundleAnalyzerPlugin {
};

if (compiler.hooks) {
compiler.hooks.done.tapAsync('webpack-bundle-analyzer', done);
compiler.hooks.compilation.tap(PLUGIN_NAME, (compilation) => {
this.logger = compilation.getLogger(PLUGIN_NAME);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, logLevel option won't work anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the launch is always from webpack, it will use webpack.config.js level setting. See https://v4.webpack.js.org/configuration/stats/#statslogging and https://webpack.js.org/configuration/stats/#statslogging

bin/analyzer still can have its own log level

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to update readme in this case and remove logLevel option from this list as it will be ignored.

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 have just realized for webpack compiler without using hooks, it will fallback to your own logger. I do not know which version is not using hooks

wood1986 marked this conversation as resolved.
Show resolved Hide resolved
});
compiler.hooks.done.tapAsync(PLUGIN_NAME, done);
} else {
compiler.plugin('done', done);
}
}

async generateStatsFile(stats) {
const statsFilepath = path.resolve(this.compiler.outputPath, this.opts.statsFilename);
await fs.promises.mkdir(path.dirname(statsFilepath), {recursive: true});
utils.mkdirpSync(this.fs, statsFilepath);

try {
await writeStats(stats, statsFilepath);
Expand Down Expand Up @@ -117,7 +122,8 @@ class BundleAnalyzerPlugin {
reportFilename: path.resolve(this.compiler.outputPath, this.opts.reportFilename || 'report.json'),
bundleDir: this.getBundleDirFromCompiler(),
logger: this.logger,
excludeAssets: this.opts.excludeAssets
excludeAssets: this.opts.excludeAssets,
fs: this.fs
});
}

Expand All @@ -129,7 +135,8 @@ class BundleAnalyzerPlugin {
bundleDir: this.getBundleDirFromCompiler(),
logger: this.logger,
defaultSizes: this.opts.defaultSizes,
excludeAssets: this.opts.excludeAssets
excludeAssets: this.opts.excludeAssets,
fs: this.fs
});
}

Expand Down
7 changes: 5 additions & 2 deletions src/bin/analyzer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const {resolve, dirname} = require('path');

const commander = require('commander');
const {magenta} = require('chalk');
const fs = require('fs');

const analyzer = require('../analyzer');
const viewer = require('../viewer');
Expand Down Expand Up @@ -138,14 +139,16 @@ if (mode === 'server') {
defaultSizes,
bundleDir,
excludeAssets,
logger: new Logger(logLevel)
logger: new Logger(logLevel),
fs
});
} else if (mode === 'json') {
viewer.generateJSONReport(bundleStats, {
reportFilename: resolve(reportFilename || 'report.json'),
bundleDir,
excludeAssets,
logger: new Logger(logLevel)
logger: new Logger(logLevel),
fs
});
}

Expand Down
8 changes: 8 additions & 0 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const {inspect, types} = require('util');
const _ = require('lodash');
const opener = require('opener');
const path = require('path');

const MONTHS = ['Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun', 'Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec'];

Expand Down Expand Up @@ -63,3 +64,10 @@ exports.open = function (uri, logger) {
logger.debug(`Opener failed to open "${uri}":\n${err}`);
}
};

exports.mkdirpSync = function mkdirpSync(fs, dest) {
// older version webpack uses memory-fs whose mkdirSync does not support {recursive: true}
fs.mkdirpSync
? fs.mkdirpSync(path.dirname(dest))
: fs.mkdirSync(path.dirname(dest), {recursive: true});
};
14 changes: 7 additions & 7 deletions src/viewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const path = require('path');
const fs = require('fs');
wood1986 marked this conversation as resolved.
Show resolved Hide resolved
const http = require('http');

const WebSocket = require('ws');
Expand All @@ -9,7 +8,7 @@ const {bold} = require('chalk');

const Logger = require('./Logger');
const analyzer = require('./analyzer');
const {open} = require('./utils');
const {open, mkdirpSync} = require('./utils');
const {renderViewer} = require('./template');

const projectRoot = path.resolve(__dirname, '..');
Expand Down Expand Up @@ -129,7 +128,8 @@ async function generateReport(bundleStats, opts) {
bundleDir = null,
logger = new Logger(),
defaultSizes = 'parsed',
excludeAssets = null
excludeAssets = null,
fs
} = opts || {};

const chartData = getChartData({logger, excludeAssets}, bundleStats, bundleDir);
Expand All @@ -145,7 +145,7 @@ async function generateReport(bundleStats, opts) {
});
const reportFilepath = path.resolve(bundleDir || process.cwd(), reportFilename);

fs.mkdirSync(path.dirname(reportFilepath), {recursive: true});
mkdirpSync(fs, reportFilepath, reportHtml);
fs.writeFileSync(reportFilepath, reportHtml);

logger.info(`${bold('Webpack Bundle Analyzer')} saved report to ${bold(reportFilepath)}`);
Expand All @@ -156,14 +156,14 @@ async function generateReport(bundleStats, opts) {
}

async function generateJSONReport(bundleStats, opts) {
const {reportFilename, bundleDir = null, logger = new Logger(), excludeAssets = null} = opts || {};
const {reportFilename, bundleDir = null, logger = new Logger(), excludeAssets = null, fs} = opts || {};

const chartData = getChartData({logger, excludeAssets}, bundleStats, bundleDir);

if (!chartData) return;

await fs.promises.mkdir(path.dirname(reportFilename), {recursive: true});
await fs.promises.writeFile(reportFilename, JSON.stringify(chartData));
mkdirpSync(fs, reportFilename);
fs.writeFileSync(reportFilename, JSON.stringify(chartData));

logger.info(`${bold('Webpack Bundle Analyzer')} saved JSON report to ${bold(reportFilename)}`);
}
Expand Down
76 changes: 46 additions & 30 deletions test/dev-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,42 +1,58 @@
const fs = require('fs');
const {spawn} = require('child_process');

const del = require('del');

const ROOT = `${__dirname}/dev-server`;
const WEBPACK_CONFIG_PATH = `${ROOT}/webpack.config.js`;
const webpackConfig = require(WEBPACK_CONFIG_PATH);
const DevServer = require('webpack-dev-server');
const webpack = require('webpack');

describe('Webpack Dev Server', function () {
beforeAll(deleteOutputDirectory);
afterEach(deleteOutputDirectory);

const timeout = 15000;
jest.setTimeout(timeout);

it('should save report file to the output directory', function (done) {
const startedAt = Date.now();

const devServer = spawn(`${__dirname}/../node_modules/.bin/webpack-dev-server`, ['--config', WEBPACK_CONFIG_PATH], {
cwd: ROOT
it('should save report file to memory file system when writeToDisk is empty', async function () {
expect.assertions(2);
const compiler = webpack(webpackConfig);
const devServer = await new Promise((resolve) => {
const devServerOptions = {host: '127.0.0.1', port: 8080};
const devServer = new DevServer(compiler, devServerOptions);
devServer.listen(devServerOptions.port, devServerOptions.host, () => {
resolve(devServer);
});
});
await new Promise((resolve) => {
compiler.hooks.afterDone.tap('webpack-bundle-analyzer', resolve);
});
const path = `${webpackConfig.output.path}/report.html`;
expect(compiler.outputFileSystem.existsSync(path)).toBeTruthy();
expect(fs.existsSync(path)).toBeFalsy();
compiler.outputFileSystem.unlinkSync(path);
await new Promise((resolve) => {
devServer.close(() => {
resolve();
});
});

const reportCheckIntervalId = setInterval(() => {
if (fs.existsSync(`${webpackConfig.output.path}/report.html`)) {
finish();
} else if (Date.now() - startedAt > timeout - 1000) {
finish(`report file wasn't found in "${webpackConfig.output.path}" directory`);
}
}, 300);
});

function finish(errorMessage) {
clearInterval(reportCheckIntervalId);
devServer.kill();
done(errorMessage ? new Error(errorMessage) : null);
}
it.skip('should save report file to the output directory when writeToDisk is true', async function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost complete. This is the test I was talking about in our long conversation with @alexander-akait. And I need some help for this one. Currently, I skip because I do not want to waste the time in the build machine.

Copy link
Member

Choose a reason for hiding this comment

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

What is the problem here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I specific devServer: { writeToDisk: true } in the webpack.config.js, it should output to disk. But it is still in the memoryFS because of the following, it always go to the last else statement

image

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 have just figured out the reason why it always output to MemoryFileSystem when {writeToDisk: true}. webpack-dev-middleware is using compiler.hooks.assetEmitted when a file is assetEmitted to MemoryFileSystem, it will also emit to disk. But webpack-bundle-analyzer is not emitting the report.html or stats.json via the compiler.emitAssets So it never output to disk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The correct logic should be this

this.fs = compiler.webpack ? compiler.outputFileSystem : require('fs');

if ((process.env.WEBPACK_DEV_SERVER === 'true' || process.env.WEBPACK_SERVE === 'true')
  && (compiler.options.devServer?.writeToDisk || compiler.options.devServer?.devMiddleware?.writeToDisk)) {
  this.fs = require('fs');
}

Copy link
Member

Choose a reason for hiding this comment

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

No, it is not problem with webpack-bundle-analyzer, webpack-dev-middleware for the writeToDisk option should create layer fs, it is already tracked, no need to change something here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then how to get {writeToDisk: true} works if it is not the problem?

expect.assertions(2);
const compiler = webpack(webpackConfig);
const devServer = await new Promise((resolve) => {
const devServerOptions = {host: '127.0.0.1', port: 8080, writeToDisk: true};
const devServer = new DevServer(compiler, devServerOptions);
devServer.listen(devServerOptions.port, devServerOptions.host, () => {
resolve(devServer);
});
});
await new Promise((resolve) => {
compiler.hooks.afterDone.tap('webpack-bundle-analyzer', resolve);
});
const path = `${webpackConfig.output.path}/report.html`;
expect(compiler.outputFileSystem.existsSync(path)).toBeTruthy();
expect(fs.existsSync(path)).toBeTruthy();
compiler.outputFileSystem.unlinkSync(path);
return await new Promise((resolve) => {
devServer.close(() => {
resolve();
});
});
});
});

function deleteOutputDirectory() {
del.sync(webpackConfig.output.path);
}