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

feat: allow --reporter to be relative to CWD #24

Merged
merged 5 commits into from
Jun 25, 2018

Conversation

elliottsj
Copy link
Contributor

This PR contains a:

  • new feature
  • test update

Motivation / Use-Case

Previously, the --reporter path must have been either absolute or in node_modules or relative to lib/compiler.js.

This change ensures that a path passed to --reporter is resolved relative to process.cwd().

e.g. you can now run this inside of a project directory to use a custom reporter:

webpack --reporter ./MyCustomReporter.js

Breaking Changes

None

This test ensures that a --reporter value can be passed with a path
relative to the CWD.

The test should fail with this error:

      1) Commands
          should accept custom reporters relative to the current working directory:
        Command failed: /Users/spencerelliott/Dev/elliottsj/webpack-command/lib/cli.js --reporter ../../lib/reporters/BasicReporter /Users/spencerelliott/Dev/elliottsj/webpack-command/test/fixtures/flags/config/src
    TypeError: ReporterClass is not a constructor
        at module.exports (/Users/spencerelliott/Dev/elliottsj/webpack-command/lib/compiler.js:91:20)
        at load.then (/Users/spencerelliott/Dev/elliottsj/webpack-command/lib/index.js:45:47)
        at process._tickCallback (internal/process/next_tick.js:68:7)
        at Function.Module.runMain (internal/modules/cjs/loader.js:746:11)
        at startup (internal/bootstrap/node.js:238:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:572:3)
@jsf-clabot
Copy link

jsf-clabot commented Jun 2, 2018

CLA assistant check
All committers have signed the CLA.

@elliottsj elliottsj changed the title Custom reporter cwd feat: ensure --reporter can be relative to CWD Jun 2, 2018
Previously, the --reporter path must have been either absolute or in
node_modules or relative to lib/compiler.js.

This change ensures that a path passed to --reporter is resolved relative
to process.cwd().

e.g. you can now run this inside of a project directory to use a custom
reporter:

    webpack --reporter ./MyCustomReporter.js
@elliottsj elliottsj changed the title feat: ensure --reporter can be relative to CWD feat: allow --reporter to be relative to CWD Jun 2, 2018
@elliottsj elliottsj force-pushed the custom-reporter-cwd branch from 0ae7fe2 to a2167b3 Compare June 2, 2018 20:22
Copy link
Contributor

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

Love the idea. But I'm not a fan of the code changes at all. Not saying that to be harsh in the slightest - the refactor can be much, much better. The tests are solid, but the increase in complexity of the code is no bueno.

This block was originally designed to be very simple, and the try/catch approach was pretty basic; one require fails, try another without having the inspect the filesystem, and if all else fails, return null and it'll be handled downstream.

Throwing relative paths in there means that the target can be one of three things:

  1. reporter in the package
  2. reporter something on the filesystem
  3. reporter in node_modules

The very first thing that should be done is to check to see if the value pass matches a path. If it does, check to see if that file exists. If it doesn't exist, enter the block that existed previous to this commit. If it does exist, attempt require, bail and pass null if something goes awry.

@elliottsj
Copy link
Contributor Author

Hey, thanks for the feedback!

Before I make changes, I'd like to clarify the reporter priority you're suggesting:

  1. Treat the reporter value as a file system path, and check whether the file exists. If it's a relative path, then check it relative to CWD.
  2. If the above fails, then treat the value as the prefix in ./reporters/${prefix}Reporter. Try to require() it.
  3. If the above fails, then treat the value as a module name passed directly to require(name). This is intended to load a reporter from another npm package.
  4. If the above fails, then return null.

I can do the refactor as you described, keeping the original requireReporter function and defining a new function (e.g. requireReporterPath) which checks the path first, then calls requireReporter.

IMO, having a small chain of try / catch is a bit simpler than the recursive call with the local param, since it reads from top-to-bottom like the list above. But I don't feel too strongly about this 😜

@shellscape
Copy link
Contributor

@elliottsj I tweaked this just a bit to follow a different pattern that should make it easier to extend the location of reporters in the future without additional nesting.

@shellscape shellscape merged commit a5cff71 into webpack-contrib:master Jun 25, 2018
@elliottsj elliottsj deleted the custom-reporter-cwd branch June 25, 2018 17:18
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