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

Make sure dependency error reporting always works #929

Merged
merged 1 commit into from
May 6, 2017

Conversation

trygveaa
Copy link
Contributor

@trygveaa trygveaa commented May 5, 2017

In some cases, the logging from console.error is swallowed (e.g. when
using create-react-app and having multiple test files). By placing the
message in the error instead, it is shown.

Before this change and without having react-test-renderer as a dependency, this message would be shown:

    Cannot find module 'react-addons-test-utils' from 'react-compat.js'
      
      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:151:17)
      at node_modules/enzyme/build/react-compat.js:143:21
      at Object.<anonymous> (node_modules/enzyme/build/react-compat.js:214:5)

After this change, this is shown instead:

    react-dom@15.5+ and react-test-renderer are implicit dependencies when using react@15.5+ with enzyme. Please add the appropriate version to your devDependencies. See https://github.com/airbnb/enzyme#installation
      
      at Object.<anonymous> (node_modules/enzyme/build/react-compat.js:143:13)
      at Object.<anonymous> (node_modules/enzyme/build/Utils.js:65:20)
      at Object.<anonymous> (node_modules/enzyme/build/MountedTraversal.js:36:14)
      at Object.<anonymous> (node_modules/enzyme/build/ReactWrapper.js:35:25)
      at Object.<anonymous> (node_modules/enzyme/build/index.js:6:21)
      at Object.<anonymous> (src/App.test.js:2:41)
      at handle (node_modules/worker-farm/lib/child/index.js:41:8)
      at process.<anonymous> (node_modules/worker-farm/lib/child/index.js:47:3)
      at emitTwo (events.js:106:13)
      at process.emit (events.js:194:7)
      at process.nextTick (internal/child_process.js:766:12)
      at _combinedTickCallback (internal/process/next_tick.js:73:7)
      at process._tickCallback (internal/process/next_tick.js:104:9)

Is there any specific reason console.error was used in the first place?

In some cases, the logging from console.error is swallowed (e.g. when
using create-react-app and having multiple test files). By placing the
message in the error instead, it is shown.
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This seems reasonable.

@ljharb
Copy link
Member

ljharb commented May 5, 2017

(separately, if console.error is swallowed in create-react-app, that's a bug in that tool - please do file it)

@nfcampos
Copy link
Collaborator

nfcampos commented May 6, 2017

LGTM, merging

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.

3 participants