Skip to content
This repository has been archived by the owner on Sep 11, 2018. It is now read-only.

feat: add JSON reporter. fixes #16 #39

Merged
merged 5 commits into from
Jun 24, 2018
Merged

feat: add JSON reporter. fixes #16 #39

merged 5 commits into from
Jun 24, 2018

Conversation

shellscape
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Issue #16 called for a reporter that did nothing but write the JSON result of a build to stdout.

Breaking Changes

None

Additional Info

webpack-cli ships with a mind bottling number of CLI flags one can use to control the output. Those seem pretty limited for the JSON option, though it looks like webpack-cli also sets stats.modules = true for JSON output. As a practice and principle, webpack-command does not support those flags, nor does it default stats.modules if the JsonReporter is used; those settings should be controlled by users in a config.

/cc @billyjanitsch @valscion for review

/* istanbul ignore next */
progress() {
throw new WebpackCommandError(
'Build rogress display is not supported when using the JSON reporter'
Copy link

Choose a reason for hiding this comment

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

There is a typo here:
rogress instead of progress

@valscion
Copy link
Member

valscion commented Jun 24, 2018

Looks nice! I'll test this with webpack-bundle-analyzer.

Note that if the output of stats.toJson is huge, JSON.stringify could cause the process to run out of memory. We faced this over at webpack-bundle-analyzer: webpack-contrib/webpack-bundle-analyzer#119

The fix was to use bfj to write the JSON in a streaming fashion: https://github.com/webpack-contrib/webpack-bundle-analyzer/pull/129/files

I'm not sure if this will be a problem with webpack-command, though. And whether it's worth it to add another dependency here.

@shellscape
Copy link
Contributor Author

@valscion that's a great bit of info, thanks. I wonder if there are key indicators in the stats object that would determine if we needed to use bfj.

@valscion
Copy link
Member

I wonder if there are key indicators in the stats object that would determine if we needed to use bfj.

Hmm I think we could check if any of the array values are exceedingly large, and then use something like bfj?

For example, given this JSON for webpack 3:

{
  "errors": [],
  "warnings": [],
  "version": "3.11.0",
  "hash": "79b004e761ec803e461a",
  "children": [
    {
      "errors": [],
      "warnings": [],
      "hash": "79b004e761ec803e461a",
      "time": 12038,
      "publicPath": "",
      "assetsByChunkName": { },
      "filteredAssets": 0,
      "entrypoints": { },
      "chunks": [
        { }
      ],
      "modules": [
        { }
      ],
      "filteredModules": 0
    }
  ]
}

...the huge values in here can be the chunks and modules. We could add some logic to say the combined sizes of some of these are larger than some constant, then we'd use streaming JSON output.

I'm not sure if it's worth the hassle, though. I've seen the stats JSON having various kinds of structures in the past, and so peeking inside it has seemed to be a bit brittle.

@shellscape
Copy link
Contributor Author

I've seen the stats JSON having various kinds of structures in the past, and so peeking inside it has seemed to be a bit brittle.

Seems to be a trend in webpack. The same thing is true of errors and warnings - you never know what you're going to get in there.

OK so I suppose we'll chalk this up as a might-happen-case and if it does pop up, we know what to do to solve it. bfj isn't that big but it's big enough that a third-party, separate module containing a reporter for that scenario would probably be the best route given that it'll be a specialized use-case.

@valscion
Copy link
Member

Yeah, it's probably best this way. webpack-cli would also have had the same issue, I think, as that also used JSON.stringify if I remember correctly.

if it does pop up, we know what to do to solve it

Yeah, this seems to be a good approach here :)

@valscion
Copy link
Member

valscion commented Jun 24, 2018

Seems like webpack-cli outputs the results of --json prettified, i.e. are indeed indented the way I wrote in #39 (comment)

This instead outputs the entire result to one line. It makes it a bit difficult to open the resulting JSON file to manually check any of the contents. I'm not sure if it's something that's concerning here, but we did indeed have a PR over at webpack-bundle-analyzer to specifically change this to be pretty formatted: webpack-contrib/webpack-bundle-analyzer#180 — I'd suppose you would get pressure to change the output to be less condensed if kept as-is.


I tested this out on my webpack 4 migration code, and indeed with --reporter json, the resulting JSON can be used for

webpack-bundle-analyzer path/to/stats.json

so for our use cases, this feature works.


How do you feel about this CLI flag differing from webpack-cli? Do you think we could get similar main-level API for this as in webpack-cli so that webpack ecosystem could continue saying that --json flag works, or should we teach our users to use --reporter json instead if they're using webpack-command?

This is a concern to us at webpack-bundle-analyzer, as we've indeed documented how to use the --json flag in the README.md here: https://github.com/webpack-contrib/webpack-bundle-analyzer#usage-as-a-cli-utility

It would feel a bit strange to mention that there's two ways of getting JSON out of webpack command here. It's not a deal breaker, though — merely something that could be a bit confusing to users.


const Reporter = require('./Reporter');

module.exports = class JesonReporter extends Reporter {
Copy link
Member

Choose a reason for hiding this comment

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

Should this perhaps be JsonReporter instead of JesonReporter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jeez, yeah it should. I shouldn't PR so early in the morning 😆

Copy link
Member

Choose a reason for hiding this comment

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

I kinda liked JesonReporter 😄

@shellscape
Copy link
Contributor Author

Seems like webpack-cli outputs the results of --json prettified

Yeah, you're correct: https://github.com/webpack/webpack-cli/blob/master/bin/cli.js#L485
What I'd like to do then is provide a --pretty flag for those that need it. So it'd look like: --reporter json --pretty

It would feel a bit strange to mention that there's two ways of getting JSON out of webpack command here. It's not a deal breaker, though — merely something that could be a bit confusing to users.

All good points above that quote. So w-command operates on a different paradigm than w-cli. w-cli is an absolute mess of CLI options, and while --json is inherently simpler than --reporter json, we want to push users towards a standard paradigm they're going to see in other tools (mocha, jest, eslint, etc). And while it might take a while, I fully intend on w-command gaining the majority of the user-base. There's a slight learning curve there, but I believe it's worth while.

@valscion
Copy link
Member

👍 Seems reasonable to push the ecosystem towards a different paradigm than what webpack-cli had.

What I'd like to do then is provide a --pretty flag for those that need it. So it'd look like: --reporter json --pretty

Could we do the opposite? Something like --reporter json --optimized or alike — as most likely the prettification will be quite fast and will likely be what users expected to see.

I don't feel too strongly on this, though — you do what you think is best ☺️

@shellscape
Copy link
Contributor Author

I've got no issue doing pretty output first. If there's a call for non-pretty we can add that option in there at that point.

@valscion
Copy link
Member

Nice, this seems good to me!


On another note, I got this error when I accidentally ran on master branch where the json reporter wasn't defined:

TypeError: ReporterClass is not a constructor
    at module.exports (/Users/vesa/code/test/node_modules/webpack-command/lib/compiler.js:90:20)
    at load.then (/Users/vesa/code/test/node_modules/webpack-command/lib/index.js:45:47)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
    at Function.Module.runMain (module.js:686:11)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3
error Command failed with exit code 1.

Should I open a new issue to discuss adding a better error message here?

@shellscape
Copy link
Contributor Author

No need, I'll get a fix up for that here in a moment. Thanks for pointing that out!

@shellscape shellscape merged commit 76f6869 into master Jun 24, 2018
@shellscape shellscape deleted the feat/json-reporter branch June 24, 2018 16:28
@valscion
Copy link
Member

Thanks! Great work 👍

goto-bus-stop pushed a commit to goto-bus-stop/react-abstract-autocomplete that referenced this pull request Jun 25, 2018
## Version **0.3.0** of **[webpack-command](https://github.com/webpack-contrib/webpack-command)** was just published.

<table>
  <tr>
    <th align=left>
      Dependency
    </th>
    <td>
      <code>[webpack-command](https://github.com/webpack-contrib/webpack-command)</code>
    </td>
  </tr>
  <tr>
      <th align=left>
       Current Version
      </th>
      <td>
        0.2.1
      </td>
    </tr>
  <tr>
    <th align=left>
      Type
    </th>
    <td>
      devDependency
    </td>
  </tr>
</table>



The version **0.3.0** is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

It might be worth looking into these changes and trying to get this project onto the latest version of webpack-command.

If you have a solid test suite and good coverage, a passing build is a strong indicator that you can take advantage of these changes directly by merging the proposed change into your project. If the build fails or you don’t have such unconditional trust in your tests, this branch is a great starting point for you to work on the update.


---


<details>
<summary>Release Notes</summary>
<strong>v0.3.0</strong>

<h2>Bugfixes</h2>
<ul>
<li>fix: define flag documentation, run-dev and run-prod implementation (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="332770158" data-permission-text="Issue title is private" data-url="webpack-contrib/webpack-command#34" href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/pull/34">#34</a>)</li>
<li>fix: add handling for exit codes on error (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="332811830" data-permission-text="Issue title is private" data-url="webpack-contrib/webpack-command#35" href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/pull/35">#35</a>)</li>
<li>fix: context overriden by flags default value. fixes <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="333107805" data-permission-text="Issue title is private" data-url="webpack-contrib/webpack-command#36" href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/issues/36">#36</a> (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="335098804" data-permission-text="Issue title is private" data-url="webpack-contrib/webpack-command#38" href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/pull/38">#38</a>)</li>
</ul>
<h2>Features</h2>
<ul>
<li>feat: warn if bail is used in watch mode (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="324626565" data-permission-text="Issue title is private" data-url="webpack-contrib/webpack-command#21" href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/pull/21">#21</a>)</li>
<li>feat: allow --reporter to be relative to CWD (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="328774213" data-permission-text="Issue title is private" data-url="webpack-contrib/webpack-command#24" href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/pull/24">#24</a>)</li>
<li>feat: add JSON reporter. fixes <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="321608573" data-permission-text="Issue title is private" data-url="webpack-contrib/webpack-command#16" href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/issues/16">#16</a> (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="335103834" data-permission-text="Issue title is private" data-url="webpack-contrib/webpack-command#39" href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/pull/39">#39</a>)</li>
<li>feat: export <code>wp</code> binary in package to avoid binary name conflicts (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="335137725" data-permission-text="Issue title is private" data-url="webpack-contrib/webpack-command#40" href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/pull/40">#40</a>)</li>
</ul>
<h2>Updates</h2>
<p><code>webpack-command</code> is no longer just a proof of concept, but a legitimate replacement for webpack-cli.</p>
</details>

<details>
<summary>Commits</summary>
<p>The new version differs by 15 commits.</p>
<ul>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/11e7799c81c0af9d3356243d83acb75c6a5af8e2"><code>11e7799</code></a> <code>chore(release): 0.3.0</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/77e64110141c540bf38ae3f267ab0d03aa3cbde4"><code>77e6411</code></a> <code>chore: update circleci config</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/9c232b749b8abdd94022659da7b92f92cbb9a272"><code>9c232b7</code></a> <code>test: update relative path reporter test snapshot</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/a5cff710f4e02b7679597a6d9b10eb8f595525df"><code>a5cff71</code></a> <code>feat: allow --reporter to be relative to CWD (#24)</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/5786adc88548ff6fbed8831aa3cb339fa06c9d81"><code>5786adc</code></a> <code>docs: update README details</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/17a73b49512e946afb6b129f4ac7dad174ddce18"><code>17a73b4</code></a> <code>chore: eslint@5 has issues with eslint-webpack-config. downgrade</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/a31d9c511639962b7642b608d80753181d9357b8"><code>a31d9c5</code></a> <code>chore: update deps, metadata</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/19b486acf297f7b1a22af8ab65df22e343efc831"><code>19b486a</code></a> <code>test: update reporter snapshots for context fix</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/ec6406089c13590e300b1c0f5a1bb43b95df5632"><code>ec64060</code></a> <code>fix: add handling for exit codes on error (#35)</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/76f6869762dcb2b87c28d1fe780700f4efff336a"><code>76f6869</code></a> <code>feat: add JSON reporter. fixes #16 (#39)</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/5afbcee76e769026e8ef8ba9c72af7ecb91707d0"><code>5afbcee</code></a> <code>feat: export <code>wp</code> binary in package to avoid binary name conflicts (#40)</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/331eb8d39eeddde959b731d21afc68c0defcdafb"><code>331eb8d</code></a> <code>fix: context overriden by flags default value. fixes #36 (#38)</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/204b5f9ee381813ea3e7d35681ab153eccf3c196"><code>204b5f9</code></a> <code>feat: warn if bail is used in watch mode (#21)</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/32d4f63e47fe459ac3cb6d40a477f148a5b768e1"><code>32d4f63</code></a> <code>docs: remove "proof of concept" from description</code></li>
<li><a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/commit/75ff73c1389f1c4575318116185d8bbe8b7aa515"><code>75ff73c</code></a> <code>fix: define flag documentation, run-dev and run-prod implementation (#34)</code></li>
</ul>
<p>See the <a href="https://urls.greenkeeper.io/webpack-contrib/webpack-command/compare/63c94a347cc67f3d14b6e99d9416a0c6866751b8...11e7799c81c0af9d3356243d83acb75c6a5af8e2">full diff</a></p>
</details>

<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants