Make "weak" optional dependency and check it at runtime #4984
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4984 +/- ##
=========================================
+ Coverage 60.27% 60.3% +0.02%
=========================================
Files 198 198
Lines 6659 6661 +2
Branches 3 3
=========================================
+ Hits 4014 4017 +3
+ Misses 2645 2644 -1
Continue to review full report at Codecov.
|
| try { | ||
| weak = require('weak'); | ||
| } catch (err) { | ||
| throw new Error( |
There was a problem hiding this comment.
This hides all sorts of errors. can we do an explicit check for MODULE_NOT_FOUND, and if not, rethrow original error?
(can use https://www.npmjs.com/package/optional and rethrow, but rolling our own should not be an issue)
There was a problem hiding this comment.
for convenience, optional would be something like
const weak = optional('weak', { rethrow: true });
if (!weak) {
throw new Error(
'The leaking detection mechanism requires the "weak" package to work. ' +
'Please make sure that you can install the native dependency on your platform.',
);
}current usage is just an if away, so might not be worth the extra dep 🙂
There was a problem hiding this comment.
I like the stricter check, let me add that into a new PR but as our own custom check; checking error.code should not require a new dependency 🙂
* Revert "docs: update expect.anything() example (#5007)" This reverts commit ea3fabc. * Revert "Emphasise required return (#4999)" This reverts commit 4f1113c. * Revert "Makes NPM a link like Yarn is (#4998)" This reverts commit aa4f09d. * Revert "Update Timeout error message to `jest.timeout` and display current timeout value (#4990)" This reverts commit 08f8394. * Revert "fix: jest-util should not depend on jest-mock (#4992)" This reverts commit 4e2f41f. * Revert "Update Troubleshooting.md (#4988)" This reverts commit 6414f28. * Revert "Revert "Add the Yammer logo to the 'who is using this' section of the website." (#4987)" This reverts commit 91b104f. * Revert "Make "weak" optional dependency and check it at runtime (#4984)" This reverts commit e00529d. * Revert "Re-inject native Node modules (#4970)" This reverts commit ef55e89.
|
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. |
On systems that do not support Node bindings, adding
weakas a dependency will make it crash. Since most of the timesjest-leak-detectoris not used at all, making the dependency optional and delaying its instantiation will fix the issue.Tested by running
./jest jest-leak-detector.