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

Whitelist DEBUG_FD for values 1 and 2 only Fixes #410 #415

Merged
merged 4 commits into from Jan 24, 2017

Conversation

@pi0
Copy link
Contributor

@pi0 pi0 commented Jan 22, 2017

UPDATED: Please see discussion below

Short description

This PR tries to smartly hide DEBUG_FD deprecation annoying warning if this conditions are met:

  • DEBUG_COLORS env variable is detected (webstorm uses that)
  • DEBUG_FD is set to 1 (default value by web-storm & idea)

So if user customizes DEBUG_FD to something other than 1 she will still get the notification but this saves majority of other users

Long Story

As discussed in #410 Also Here and Here after DEBUG_FD is deprecated many & many end users who actually don't even use DEBUG_FD variable are affected (including all express& react-native users). This issue will not be resolved at lease next idea release (end of march) also if it resolves in that way webstorm users will totally lose error colors!

@coveralls
Copy link

@coveralls coveralls commented Jan 22, 2017

Coverage Status

Coverage remained the same at 63.75% when pulling eb0bcad on pi0:patch-1 into 705a9fe on visionmedia:master.

1 similar comment
@coveralls
Copy link

@coveralls coveralls commented Jan 22, 2017

Coverage Status

Coverage remained the same at 63.75% when pulling eb0bcad on pi0:patch-1 into 705a9fe on visionmedia:master.

@pi0
Copy link
Contributor Author

@pi0 pi0 commented Jan 22, 2017

@TooTallNate @thebigredgeek Would you please review this PR? :)

@TooTallNate
Copy link
Member

@TooTallNate TooTallNate commented Jan 23, 2017

Thanks for the patch. I think instead of doing something like this, we're going to whitelist DEBUG_FD for values 1 and 2 only, since they can be supported in a backwards compatible way without using undocumented Node.js APIs. I believe that would avoid the needs for an explicit patch like this.

@pi0
Copy link
Contributor Author

@pi0 pi0 commented Jan 23, 2017

@TooTallNate Thanks for your review, i've changed PR so we only white-list values of 1 & 2. also link to git.io is replaced with something better :)) What's your idea?

@coveralls
Copy link

@coveralls coveralls commented Jan 23, 2017

Coverage Status

Coverage remained the same at 63.75% when pulling 5350863 on pi0:patch-1 into 705a9fe on visionmedia:master.

@pi0 pi0 changed the title Hide in DEBUG_FD deprecation warning in Webstorm Fixes #410 Whitelist DEBUG_FD for values 1 and 2 onlyn Webstorm Fixes #410 Jan 23, 2017
@pi0 pi0 changed the title Whitelist DEBUG_FD for values 1 and 2 onlyn Webstorm Fixes #410 Whitelist DEBUG_FD for values 1 and 2 only Fixes #410 Jan 23, 2017
src/node.js Outdated
var fd = parseInt(process.env.DEBUG_FD, 10) || 2;

if (1 !== fd && 2 !== fd) {
util.deprecate(function(){}, '`DEBUG_FD` is deprecated. Override `debug.log` if you want to use a different log function (https://git.io/debug_fd)')()

This comment has been minimized.

@thebigredgeek

thebigredgeek Jan 23, 2017
Collaborator

We should probably use a different message here, such as telling the user that using DEBUG_FD outside of file descriptors 1 and 2 is no longer supported or deprecated

@pi0
Copy link
Contributor Author

@pi0 pi0 commented Jan 23, 2017

@thebigredgeek Done :)

@coveralls
Copy link

@coveralls coveralls commented Jan 23, 2017

Coverage Status

Coverage remained the same at 63.75% when pulling 2a8f8a4 on pi0:patch-1 into 705a9fe on visionmedia:master.

@thebigredgeek thebigredgeek merged commit 37e14d6 into visionmedia:master Jan 24, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 63.75%
Details
@bisubus
Copy link

@bisubus bisubus commented Feb 5, 2017

@thebigredgeek Can it be published as 2.6.1, please? Packages often have strict constraints for dependency versions. They will be hard-coded to 2.6.0 eventually. This has already happened with body-parser.

@thebigredgeek
Copy link
Collaborator

@thebigredgeek thebigredgeek commented Feb 7, 2017

Hey guys, will slice a release this afternoon

@thebigredgeek
Copy link
Collaborator

@thebigredgeek thebigredgeek commented Feb 10, 2017

released 2.6.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.