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

Added JSON output to the cli and webpack plugin #341

Merged
merged 18 commits into from Apr 10, 2020
Merged

Added JSON output to the cli and webpack plugin #341

merged 18 commits into from Apr 10, 2020

Conversation

Gongreg
Copy link
Contributor

@Gongreg Gongreg commented Apr 3, 2020

Issue

This issue can be related to #194.

We would like to use the statistics generated by Webpack Bundle Analyzer to track changes in projects. For that we would like to upload/save the bundle sizes in a database, and json is a lot more flexible format for that.

Tests

I've added a test for the json output.

Next step

There is already possibility to generate html from the given json (you expose ejs file that we can directly use). But I think it would be great to add a way to generate the html from cli, but I can't think of a nice way to do it. This is what I currently have created: https://github.com/Gongreg/webpack-bundle-analyzer/pull/1/files

@jsf-clabot
Copy link

jsf-clabot commented Apr 3, 2020

CLA assistant check
All committers have signed the CLA.

@valscion
Copy link
Member

valscion commented Apr 3, 2020

Hi, thanks for opening a pull request for this ☺️. Gotta say that this is a surprisingly small changeset! I expected this to need more code.

I wonder if @th0r has opinions about letting people save the generated chart data as JSON instead of HTML with a new option?

This approach taken here seems quite legit to me!

@Gongreg
Copy link
Contributor Author

Gongreg commented Apr 3, 2020

Glad to hear that!

If you will decide to include this functionality and there are any changes needed for it please let me know :)

@valscion
Copy link
Member

valscion commented Apr 3, 2020

Will do :)

Copy link
Member

@valscion valscion left a comment

Choose a reason for hiding this comment

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

I think it would be good for the ecosystem to have this kind of feature in webpack-bundle-analyzer ☺️. So here's the first full review pass from me ☺️

If there's something in my review comments that isn't clear, feel free to comment saying so 😊. It's also OK to disagree with my inline comments and reply with an alternative viewpoint 💞

src/bin/analyzer.js Outdated Show resolved Hide resolved
src/bin/analyzer.js Outdated Show resolved Hide resolved
src/BundleAnalyzerPlugin.js Outdated Show resolved Hide resolved
src/viewer.js Outdated Show resolved Hide resolved
test/analyzer.js Outdated Show resolved Hide resolved
test/analyzer.js Outdated Show resolved Hide resolved
src/BundleAnalyzerPlugin.js Outdated Show resolved Hide resolved
src/viewer.js Outdated Show resolved Hide resolved
@Gongreg
Copy link
Contributor Author

Gongreg commented Apr 6, 2020

Hey @valscion. I've updated the PR according to the comments. Wrote one reply.

I wanted to ask about node api (I mean the exposed methods in the entry of the package). At the moment the method to start the server is exposed. Could we also expose the generateReport/generateJSONReport methods there? It would simplify using this package inside other node projects.

Also this way we could expose the generation of html from json in a proper way instead of abusing the cli (as I showed here: https://github.com/Gongreg/webpack-bundle-analyzer/pull/1/files)

I know that this would expose a lot more of internals than it used to be before, but on the other hand they are already accessible through the cli.

@valscion
Copy link
Member

valscion commented Apr 6, 2020

I'm not yet sold on the idea of exposing the generation of HTML from JSON so I'd rather not delve too deep into that subject here, if that's okay to you ☺️.

Let's get the JSON output format out the door first.

I wanted to ask about node api (I mean the exposed methods in the entry of the package). At the moment the method to start the server is exposed.

Sorry, I'm running a bit slow today — which file are you referring to here?

@Gongreg
Copy link
Contributor Author

Gongreg commented Apr 6, 2020

https://github.com/webpack-contrib/webpack-bundle-analyzer/blob/master/src/index.js

@valscion
Copy link
Member

valscion commented Apr 6, 2020

At the moment the method to start the server is exposed.

I think that's legacy API and we might even want to get rid of it. We haven't documented its existence in the README. It's been there since I joined maintaining this plugin, so I don't know what the idea behind it is.

I'd like to avoid adding any new top-level APIs here outside of the new analyzer mode option. We can of course open a discussion about the top-level API — that discussion would preferably be outside this PR, though, in its own issue ☺️

src/viewer.js Outdated Show resolved Hide resolved
@Gongreg
Copy link
Contributor Author

Gongreg commented Apr 6, 2020

Sure,
I am trying to think of a way to nicely expose both json generation functionality (seems like this PR is good enough) and html report generation from the json. This is not a common use case and 99% of the users will not use it, but for those who wish to do the generation in multiple steps (store json somewhere and render when it is needed) it is important to be able to do it.

Should I create a separate issue for it? For now we will just directly render the ejs you have in the package.

@valscion
Copy link
Member

valscion commented Apr 6, 2020

Should I create a separate issue for it?

If you want to continue the conversation about this feature then sure ☺️. You can also open a separate pull request after this PR with your example change, and use that as a discussion starting point — we make no promises to merge that PR but it does help to have some code about a use case already available when discussing it ☺️

Copy link
Member

@valscion valscion left a comment

Choose a reason for hiding this comment

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

The code looks good to me now! Thank you for the contribution ☺️

I'll give @th0r a few days to take a look at this PR if he has questions, but if not, I'll try to remember to merge and release this this week 😊

src/viewer.js Outdated Show resolved Hide resolved
@valscion valscion linked an issue Apr 6, 2020 that may be closed by this pull request
Gongreg and others added 2 commits April 6, 2020 17:34
Co-Authored-By: Vesa Laakso <482561+valscion@users.noreply.github.com>
@Gongreg
Copy link
Contributor Author

Gongreg commented Apr 10, 2020

Hey @valscion , any news? :)

@valscion valscion merged commit acf2d1c into webpack-contrib:master Apr 10, 2020
@valscion
Copy link
Member

Thanks for the contribution! I'll hopefully remember to make a release next time I'm on my coding laptop ☺️. Might be 'til end of easter, Tuesday, but might be earlier. It depends on if I want to do some coding stuff during this long weekend or not 😄

Feel free to ping me if I haven't made a release by Wednesday ☺️

@valscion
Copy link
Member

This has been released in v3.7.0 ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Generate analysis report file
3 participants