Don't format node assert errors when there's no 'assert' module #4376
Conversation
In format_node_assert_errors.js, we require the 'assert' module to detect that an error thrown is an AssertionError, so that we can format assertion errors nicely. This creates an implicit dependency in the library on the assert module (a node builtin, but not available without adding a shim in browsers, React Native, etc). If we are running in an environment where assert is not available, we don't need to try to format assertion errors, so we can just bail. Detecting this case and performing the require in a try/catch removes the implicit dependency on assert. Fixes jestjs#4365
Codecov Report
@@ Coverage Diff @@
## master #4376 +/- ##
==========================================
- Coverage 56.2% 55.82% -0.39%
==========================================
Files 191 189 -2
Lines 6421 6386 -35
Branches 6 6
==========================================
- Hits 3609 3565 -44
- Misses 2809 2818 +9
Partials 3 3
Continue to review full report at Codecov.
|
|
React Native's Metro bundler looks for all require() calls statically. The issue is that if it statically finds "require('assert')" anywhere in the code, Metro bundler will fail to build the bundle. |
|
Hmm. Does Metro have anything like webpack's require.resolveWeak? |
|
Maybe a better solution would be to pull format_node_assert_errors out so that jest-circus doesn't try to use it out of the box. We could still use |
|
What about |
|
Looks like that works (I tested via create-react-native-app); I'll change this to use that. |
The React Native packager was attempting to pull in the assert module as a
dependency when using `require('assert')`, but it does not do this if you
use require.call(null, 'assert'), because it does not detect this pattern.
|
Do you mind just getting rid of this test? It isn't really useful in its current form and I'd like to merge this fix regardless. |
|
Done |
|
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. |
Summary
In format_node_assert_errors.js, we require the 'assert' module to
detect that an error thrown is an AssertionError, so that we can format
assertion errors nicely. This creates an implicit dependency in the
library on the assert module (a node builtin, but not available without
adding a shim in browsers, React Native, etc).
If we are running in an environment where assert is not available, we
don't need to try to format assertion errors, so we can just bail.
Detecting this case and performing the require in a try/catch removes
the implicit dependency on assert.
Fixes #4365
Test plan
I have created a test that mocks assert as a module that throws when loaded, but I am not confident that the mock is installed at the right time, or that I am testing the right part of the code. I could use some direction here.