-
-
Notifications
You must be signed in to change notification settings - Fork 488
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
plugin use infrastructureLogger and output the report.html to compiler.outputFileSystem #477
base: master
Are you sure you want to change the base?
Conversation
src/BundleAnalyzerPlugin.js
Outdated
const done = (stats, callback) => { | ||
const isFromWebpackDevServer = process.env.WEBPACK_SERVE === 'true'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any other way than looking up an environment variable to figure this out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not rely on this, always use compiler.outputFileSystem
, because webpack uses compiler.outputFileSystem
to write assets, so you can read them using the same fs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @alexander-akait, I am unable to do this directly without webpack-dev-server check
this.fs = compiler.outputFileSystem
because of this
Otherwise,
webpack-bundle-analyzer/src/viewer.js
Lines 148 to 149 in 8dce252
fs.mkdirSync(path.dirname(reportFilepath), {recursive: true}); | |
fs.writeFileSync(reportFilepath, reportHtml); |
will break the tests
and this is webpack v4 specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, for webpack v5, we have these methods? I mean use compiler.outputFileSystem
for webpack v5, and keep old behavior for webpack v4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means we write file again, need debug and search where we do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to the package-lock.json, the version of memory-fs is using 0.4.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memory-fs
used only by webpack@4
, we should use require('fs')
for webpack v4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e4e9aa5
to
814bf39
Compare
I hope you won't get discouraged by the I spent a great deal of time in figuring out why the If you manage to get the tests to not hang in CI, that would be amazing. I had a pretty bad time debugging those failures myself. I cancelled the workflow run now as it was hanging. |
@valscion. I think the hanging up issue is because the second |
Even with classic |
Oh right, you removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, let's fix tests and I think we can merge
@@ -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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
devServer.kill(); | ||
done(errorMessage ? new Error(errorMessage) : null); | ||
} | ||
it.skip('should save report file to the output directory when writeToDisk is true', async function () { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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');
}
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
…compiler.outputFileSystem
@wood1986 may I kindly ask you to:
|
No problem
I will do what you ask |
fix #476