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.

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

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

Conversation

wood1986
Copy link
Contributor

@wood1986 wood1986 commented Oct 11, 2021

fix #476

src/viewer.js Show resolved Hide resolved
src/BundleAnalyzerPlugin.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
const done = (stats, callback) => {
const isFromWebpackDevServer = process.env.WEBPACK_SERVE === 'true';
Copy link
Member

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?

Copy link
Member

@alexander-akait alexander-akait Oct 11, 2021

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

Copy link
Contributor Author

@wood1986 wood1986 Oct 11, 2021

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

image

Otherwise,

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

will break the tests

and this is webpack v4 specific

Copy link
Member

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

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 see

Copy link
Member

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

Copy link
Contributor Author

@wood1986 wood1986 Oct 11, 2021

Choose a reason for hiding this comment

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

the implementation of MemoryFS does not support recursively create folder. See this
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.

according to the package-lock.json, the version of memory-fs is using 0.4.1

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I did what you have suggested and fixed the file already exist issue

I still need you help for the test. I have updated dev-server.js. For some reason, I am unable to get the test passed for {writeToDisk: true}.

image

It always use MemoryFileSystem and I do not know how to fix

@wood1986 wood1986 force-pushed the nicetohave branch 2 times, most recently from e4e9aa5 to 814bf39 Compare October 11, 2021 18:18
@valscion
Copy link
Member

valscion commented Oct 12, 2021

I hope you won't get discouraged by the dev-server tests hanging when running in GitHub actions CI 😓.

I spent a great deal of time in figuring out why the dev-server related tests were always hanging in CI and not on my own machine and the only fix that seemed to work was this one: 55f3478. If you check 36e960b and the commits leading up to it, you can see my failed attempts at figuring out different alternatives which just didn't seem to work in CI.

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.

@wood1986
Copy link
Contributor Author

wood1986 commented Oct 12, 2021

@valscion. I think the hanging up issue is because the second expect is failed. Then it stops executing the rest. If it can execute to the end, it will not hang up and finish quickly.

@wood1986
Copy link
Contributor Author

Even with classic function (done) it will still block from executing to the end if one of the expect is failed. Then it will still leave the never ended server. Jest cannot exit

@valscion
Copy link
Member

Oh right, you removed the jest.setTimeout part.

Copy link
Member

@alexander-akait alexander-akait left a 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

.gitignore Outdated Show resolved Hide resolved
src/BundleAnalyzerPlugin.js Outdated Show resolved Hide resolved
@@ -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

src/BundleAnalyzerPlugin.js Show resolved Hide resolved
src/viewer.js Outdated Show resolved Hide resolved
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?

@th0r
Copy link
Collaborator

th0r commented Oct 13, 2021

@wood1986 may I kindly ask you to:

  1. Fix PR review issues in a separate commits and don't use force-push as it's really difficult to track changes you've made
  2. Don't resolve my comments - I would like to do this myself.

@wood1986
Copy link
Contributor Author

@wood1986 may I kindly ask you to:

  1. Fix PR review issues in a separate commits and don't use force-push as it's really difficult to track changes you've made
  2. Don't resolve my comments - I would like to do this myself.

No problem

  1. I thought this is a very tiny and it should be very quick to check all the check again
  2. I usually use the resolve to let comment owner know I have completed. Meanwhile it will be easy for me to see what things left.

I will do what you ask

@alexander-akait
Copy link
Member

@th0r @valscion What we need to finish it?

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.

[Nice to Have] plugin should use webpack InfrastructureLogger and output report to compiler.outputFileSystem
4 participants