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

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 jsf-clabot commented Apr 3, 2020

CLA assistant check
All committers have signed the CLA.

Gongreg added 4 commits Apr 3, 2020
@valscion
Copy link
Member

@valscion 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 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 valscion commented Apr 3, 2020

Will do :)

Copy link
Member

@valscion valscion left a comment

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 and others added 7 commits Apr 6, 2020
Co-Authored-By: Vesa Laakso <482561+valscion@users.noreply.github.com>
…zer into json-output
@Gongreg
Copy link
Contributor Author

@Gongreg 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 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?

@valscion
Copy link
Member

@valscion 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 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 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

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 that may be closed by this pull request Apr 6, 2020
Gongreg and others added 2 commits Apr 6, 2020
Co-Authored-By: Vesa Laakso <482561+valscion@users.noreply.github.com>
@Gongreg
Copy link
Contributor Author

@Gongreg Gongreg commented Apr 10, 2020

Hey @valscion , any news? :)

@valscion valscion merged commit acf2d1c into webpack-contrib:master Apr 10, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@valscion
Copy link
Member

@valscion valscion commented Apr 10, 2020

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

@valscion valscion commented Apr 14, 2020

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
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.