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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: properly reprint resolver errors in watch mode #6407

Merged
merged 7 commits into from
Jul 7, 2018

Conversation

thymikee
Copy link
Collaborator

@thymikee thymikee commented Jun 6, 2018

Summary

We've reprinted only error.stack while being in watch mode, which caused "Could not locate module" errors to vanish, because they had their stack cleared (for reasons known to me at the time of writing, which are not relevant now).

Also improved the resolver error message just a bit.

Fixes #4524

Test plan

No time for tests because I'm heading off on vacation 馃槑馃尨, but hey, I have screenshots!

Before:
screen shot 2018-06-06 at 18 14 43

After (watch mode resolver errors now shine as never before):
screen shot 2018-06-06 at 18 08 39]

To get rid of this extra "Error: " I'd need to only print error.message which I think is safer not to do, as proven by error.stack version, but maybe I worry to much (just no time for thorough testing).

The regular run is unaffected:
screen shot 2018-06-06 at 18 13 14

@SimenB
Copy link
Member

SimenB commented Jun 6, 2018

You can do error.name = '' to get rid of the Error: prefix.

Enjoy your vacation!

@thymikee
Copy link
Collaborator Author

thymikee commented Jun 6, 2018

Thank you sir!
fixd
screen shot 2018-06-06 at 18 21 04

@SimenB
Copy link
Member

SimenB commented Jun 6, 2018

Haha!

We could potentially do error.name = 'Configuration Error' etc, but this should be fine

@codecov-io
Copy link

codecov-io commented Jun 6, 2018

Codecov Report

Merging #6407 into master will decrease coverage by <.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6407      +/-   ##
==========================================
- Coverage   63.48%   63.47%   -0.01%     
==========================================
  Files         227      227              
  Lines        8697     8699       +2     
  Branches        4        4              
==========================================
+ Hits         5521     5522       +1     
- Misses       3175     3176       +1     
  Partials        1        1
Impacted Files Coverage 螖
packages/jest-resolve-dependencies/src/index.js 97.61% <酶> (酶) 猬嗭笍
packages/jest-cli/src/watch.js 79.02% <0%> (酶) 猬嗭笍
packages/jest-resolve/src/index.js 95.96% <83.33%> (-0.76%) 猬囷笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 949a281...e631fb9. Read the comment docs.

@thymikee thymikee requested a review from cpojer June 16, 2018 19:55
@thymikee thymikee requested a review from SimenB July 6, 2018 15:33
@SimenB
Copy link
Member

SimenB commented Jul 7, 2018

CI is unhappy

@thymikee
Copy link
Collaborator Author

thymikee commented Jul 7, 2018

updated snapshot, should be green now

@cpojer cpojer merged commit ff44548 into jestjs:master Jul 7, 2018
calebeby pushed a commit to Pigmice2733/scouting-frontend that referenced this pull request Jul 11, 2018
This Pull Request updates dependency [jest](https://github.com/facebook/jest) from `v23.3.0` to `v23.4.0`



<details>
<summary>Release Notes</summary>

### [`v23.4.0`](https://github.com/facebook/jest/blob/master/CHANGELOG.md#&#8203;2340)
[Compare Source](jestjs/jest@v23.3.0...v23.4.0)
##### Features

- `[jest-haste-map]` Add `computeDependencies` flag to avoid opening files if not needed ([#&#8203;6667](`jestjs/jest#6667))
- `[jest-runtime]` Support `require.resolve.paths` ([#&#8203;6471](`jestjs/jest#6471))
- `[jest-runtime]` Support `paths` option for `require.resolve` ([#&#8203;6471](`jestjs/jest#6471))
##### Fixes

- `[jest-runner]` Force parallel runs for watch mode, to avoid TTY freeze ([#&#8203;6647](`jestjs/jest#6647))
- `[jest-cli]` properly reprint resolver errors in watch mode ([#&#8203;6407](`jestjs/jest#6407))
- `[jest-cli]` Write configuration to stdout when the option was explicitly passed to Jest ([#&#8203;6447](`jestjs/jest#6447))
- `[jest-cli]` Fix regression on non-matching suites ([6657](`jestjs/jest#6657))
- `[jest-runtime]` Roll back `micromatch` version to prevent regression when matching files ([#&#8203;6661](`jestjs/jest#6661))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
zetlen added a commit to magento/pwa-studio that referenced this pull request Jul 11, 2018
- Updated Jest dep to `^23.4.0`, fixing jestjs/jest#6407 which caused our `test:dev` script to appear to hang when files were modified and Jest cache was absent. The original behavior froze at this message:

       Determining test suites to run...

  The new version of Jest properly prints the error message:

       Determining test suites to run...

       Configuration error:

       Could not locate module src/store/reducers/navigation mapped as:
       /Users/jzetlen/gits/pwa/pwa-studio/packages/venia-concept/src/store/reducers/navigation.

       Please check your configuration for these entries:
       {
         "moduleNameMapper": {
           "/^src\/(.+)/": "/Users/jzetlen/gits/pwa/pwa-studio/packages/venia-concept/src/$1"
         },
         "resolver": null
       }

- Removed the `App` component, which is now unused due to the `MagentoResolver` calling `RootComponents` directly, and was the source of the mapping error above. Future errors of this kind will appear early and should be easier to diagnose and fix.
jimbo pushed a commit to magento/pwa-studio that referenced this pull request Jul 12, 2018
- Updated Jest dep to `^23.4.0`, fixing jestjs/jest#6407 which caused our `test:dev` script to appear to hang when files were modified and Jest cache was absent. The original behavior froze at this message:

       Determining test suites to run...

  The new version of Jest properly prints the error message:

       Determining test suites to run...

       Configuration error:

       Could not locate module src/store/reducers/navigation mapped as:
       /Users/jzetlen/gits/pwa/pwa-studio/packages/venia-concept/src/store/reducers/navigation.

       Please check your configuration for these entries:
       {
         "moduleNameMapper": {
           "/^src\/(.+)/": "/Users/jzetlen/gits/pwa/pwa-studio/packages/venia-concept/src/$1"
         },
         "resolver": null
       }

- Removed the `App` component, which is now unused due to the `MagentoResolver` calling `RootComponents` directly, and was the source of the mapping error above. Future errors of this kind will appear early and should be easier to diagnose and fix.
@thymikee thymikee deleted the fix/watch-resolver-errors branch March 16, 2019 10:56
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
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.

Potential issue with Jest with moduleNameMapper
6 participants