- Notifications
You must be signed in to change notification settings - Fork744
Silence potentially upcoming circular dependency warning#973
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Node.js is currently considering printing a warning when a non-existentproperty of `module.exports` is accessed while in a circular `require()`dependency, in order to make it easier to catch issues with circulardependencies.In order to avoid printing these warnings for shelljs, checking for theproperty’s existence rather than its truthiness suffices.Refs:nodejs/node#29935
addaleax commentedOct 18, 2019
Also: I’d understand if you don’t like this PR, because the previous code was perfectly fine. I’m trying to implement this warning in Node.js because I believe that, in the big picture, it helps debug hard problems more than it annoy people – I’d guess that circular dependencies might have tripped you up in the past, too. If you have any feedback about the Node.js PR in general, please feel free to voice it there. |
codecov-io commentedOct 18, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #973 +/- ##======================================= Coverage 97.14% 97.14% ======================================= Files 34 34 Lines 1298 1298 ======================================= Hits 1261 1261 Misses 37 37
Continue to review full report at Codecov.
|
nfischer left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This change seems fine, I don't think we need to be worried about inheritance.
Node.js is currently considering printing a warning when a non-existentproperty of `module.exports` is accessed while in a circular `require()`dependency, in order to make it easier to catch issues with circulardependencies.In order to avoid printing these warnings for shelljs, checking for theproperty’s existence rather than its truthiness suffices.Refs:nodejs/node#29935
shelljs/shelljs#973 updated shelljs to preventNode 14 from emitting circular dependency warnings.
This adds testing for node v14. This removes testing for node v6 and v7because codecov breaks on these versions. This omits node v15 becauseappveyor doesn't seem to support this version. The nodejs org currentlysupports [10, 12, 14, 15].This makes a couple minor edits to the check-node-support script forconsistency withshelljs/shx#186.This bumps the shx dependency because it seems we're hitting issue#973on node v14.Test: npm run check-node-support
This adds testing for node v14. This removes testing for node v6 and v7because codecov breaks on these versions. This omits node v15 becauseappveyor doesn't seem to support this version. The nodejs org currentlysupports [10, 12, 14, 15].This makes a couple minor edits to the check-node-support script forconsistency withshelljs/shx#186.This bumps the shx dependency because it seems we're hitting issue#973on node v14.Test: npm run check-node-support
Node.js is currently considering printing a warning when a non-existent
property of
module.exportsis accessed while in a circularrequire()dependency, in order to make it easier to catch issues with circular
dependencies.
In order to avoid printing these warnings for shelljs, checking for the
property’s existence rather than its truthiness suffices.
Refs:nodejs/node#29935