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

doc: deprecate type coercion for dns.lookup options #38906

Closed
wants to merge 4 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jun 2, 2021

The plan is to replace this check:

if (typeof options.verbatim === 'boolean') {
verbatim = options.verbatim === true;
}

with a more robust validateBoolean check – and similar for other dns.lookup options.

@github-actions github-actions bot added deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. labels Jun 2, 2021
doc/api/deprecations.md Outdated Show resolved Hide resolved
@aduh95 aduh95 added the review wanted PRs that need reviews. label Jun 11, 2021
@aduh95 aduh95 force-pushed the deprecate-non-bool-verbatim branch from 34fa6fa to 82ffb69 Compare June 11, 2021 14:35
@aduh95 aduh95 force-pushed the deprecate-non-bool-verbatim branch from 82ffb69 to 713319c Compare June 11, 2021 14:35
@aduh95 aduh95 changed the title doc: deprecate using non-boolean values in the verbatim option doc: deprecate type coercion for dns.lookup options Jun 11, 2021
@aduh95 aduh95 added the notable-change PRs with changes that should be highlighted in changelogs. label Jun 11, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Jun 11, 2021

@nodejs/tsc if #37931 lands, the type coercion on the verbatim option could be a bit surprising as before any falsy value were interpreted as false – because that's still the default value. If the default changes, a falsy value that is not false will be interpreted as true – which could be a bit surprising.

Other dns.lookup options have the behavior, which could be problematic as well. E.g., dns.lookup('localhost', { family: 'ipv4' }), the family value here is simply ignored and is silently converted to 0 – which again, should cause issues if/when verbatim default value changes, as localhost will point to ::1 instead of 127.0.0.1.
My suggestion is to apply the same logic as we do in other part of the code base, which is to enforce types for dns.lookup options. I'm not sure if this kind of change deserve to go through a deprecation cycle, or if it can simply land as a semver-major.

For reference, here's the current logic:

node/lib/dns.js

Lines 115 to 120 in fa1a842

hints = options.hints >>> 0;
family = options.family >>> 0;
all = options.all === true;
if (typeof options.verbatim === 'boolean') {
verbatim = options.verbatim === true;
}

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 16, 2021
@@ -2786,6 +2786,21 @@ These properties are now available within the standard `detail` property
of the `PerformanceEntry` object. The existing accessors have been
deprecated and should no longer be used.

### DEP0153: `dns.lookup` and `dnsPromises.lookup` options type coercion
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm landing this now, but in general deprecation PRs should leave the code number unassigned (e.g. use DEP0XXX) so that the code can be assigned when it is landed.

@jasnell
Copy link
Member

jasnell commented Aug 17, 2021

Landed in fae352a...f41893e

@jasnell jasnell closed this Aug 17, 2021
jasnell pushed a commit that referenced this pull request Aug 17, 2021
PR-URL: #38906
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
jasnell pushed a commit that referenced this pull request Aug 17, 2021
PR-URL: #38906
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@aduh95 aduh95 deleted the deprecate-non-bool-verbatim branch August 17, 2021 17:26
targos pushed a commit that referenced this pull request Aug 22, 2021
PR-URL: #38906
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Aug 22, 2021
PR-URL: #38906
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos added a commit that referenced this pull request Aug 25, 2021
Notable changes:

doc:
  * deprecate type coercion for `dns.lookup` options (Antoine du Hamel) #38906
stream:
  * (SEMVER-MINOR) add `stream.Duplex.from` utility (Robert Nagy) #39519
  * (SEMVER-MINOR) add `isDisturbed` helper (Robert Nagy) #39628
util:
  * (SEMVER-MINOR) expose `toUSVString` (Robert Nagy) #39814

PR-URL: #39875
targos added a commit that referenced this pull request Aug 25, 2021
Notable changes:

doc:
  * deprecate type coercion for `dns.lookup` options (Antoine du Hamel) #38906
stream:
  * (SEMVER-MINOR) add `stream.Duplex.from` utility (Robert Nagy) #39519
  * (SEMVER-MINOR) add `isDisturbed` helper (Robert Nagy) #39628
util:
  * (SEMVER-MINOR) expose `toUSVString` (Robert Nagy) #39814

PR-URL: #39875
wwwzbwcom pushed a commit to wwwzbwcom/node that referenced this pull request Aug 26, 2021
Notable changes:

doc:
  * deprecate type coercion for `dns.lookup` options (Antoine du Hamel) nodejs#38906
stream:
  * (SEMVER-MINOR) add `stream.Duplex.from` utility (Robert Nagy) nodejs#39519
  * (SEMVER-MINOR) add `isDisturbed` helper (Robert Nagy) nodejs#39628
util:
  * (SEMVER-MINOR) expose `toUSVString` (Robert Nagy) nodejs#39814

PR-URL: nodejs#39875
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. notable-change PRs with changes that should be highlighted in changelogs. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants