Silence potentially upcoming circular dependency warning #973
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Node.js is currently considering printing a warning when a non-existent property of `module.exports` is accessed while in a circular `require()` 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
|
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 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
approved these changes
Oct 20, 2019
nfischer
pushed a commit
that referenced
this pull request
Apr 25, 2020
Node.js is currently considering printing a warning when a non-existent property of `module.exports` is accessed while in a circular `require()` 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
This was referenced Apr 27, 2020
nfischer
pushed a commit
to shelljs/shx
that referenced
this pull request
Aug 4, 2020
shelljs/shelljs#973 updated shelljs to prevent Node 14 from emitting circular dependency warnings.
nfischer
added a commit
that referenced
this pull request
Oct 26, 2020
This adds testing for node v14. This removes testing for node v6 and v7 because codecov breaks on these versions. This omits node v15 because appveyor doesn't seem to support this version. The nodejs org currently supports [10, 12, 14, 15]. This makes a couple minor edits to the check-node-support script for consistency with shelljs/shx#186. This bumps the shx dependency because it seems we're hitting issue #973 on node v14. Test: npm run check-node-support
nfischer
added a commit
that referenced
this pull request
Oct 26, 2020
This adds testing for node v14. This removes testing for node v6 and v7 because codecov breaks on these versions. This omits node v15 because appveyor doesn't seem to support this version. The nodejs org currently supports [10, 12, 14, 15]. This makes a couple minor edits to the check-node-support script for consistency with shelljs/shx#186. This bumps the shx dependency because it seems we're hitting issue #973 on node v14. Test: npm run check-node-support
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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