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

Add dnsLookupIpVersion option #1264

Merged
merged 26 commits into from Jun 1, 2020
Merged

Add dnsLookupIpVersion option #1264

merged 26 commits into from Jun 1, 2020

Conversation

Giotino
Copy link
Collaborator

@Giotino Giotino commented May 14, 2020

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.

I've added the documentation relative to the family property. I've also added a stricter type definition.

I think the name "family" it's a little generic, it might be renamed to "IPfamily" or something like that.

Fixes #1263

Added explicit type (not inherited from Node.JS types)
@Giotino Giotino changed the title Documentation for family option Type and documentation for family option May 14, 2020
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
source/core/index.ts Outdated Show resolved Hide resolved
source/core/index.ts Outdated Show resolved Hide resolved
Added a note about the `family` option in the README
Some cleanups
@Giotino
Copy link
Collaborator Author

Giotino commented May 14, 2020

I've added some IPv6 (and DNS) related tests.

It seems like Travis CI is missing IPv6 support, event on loopback.

@Giotino
Copy link
Collaborator Author

Giotino commented May 14, 2020

Since family (now ipVersion) is only related to DNS I think it should be renamed to something like dnsIpVersion or resolvedIpVersion.

readme.md Show resolved Hide resolved
source/core/utils/dns-ip-version.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Can you also add a util.deprecate call if family option is used?

@Giotino
Copy link
Collaborator Author

Giotino commented May 16, 2020

Can you also add a util.deprecate call if family option is used?

According to the Node.JS documentation util.deprecrate is made for functions.
https://nodejs.org/api/util.html#util_util_deprecate_fn_msg_code

@sindresorhus
Copy link
Owner

There are benefits to util.deprecate. It only prints the warning once and it's also possible to silence it. Maybe we could make an utility that works the same way?

[...] that will emit a DeprecationWarning using the 'warning' event. The warning will be emitted and printed to stderr the first time the returned function is called. After the warning is emitted, the wrapped function is called without emitting a warning.

@Giotino
Copy link
Collaborator Author

Giotino commented May 16, 2020

I'm working on the "deprecation utilty" in this PR #1255
When it's ok I'll merge it here.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
Co-authored-by: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
@Giotino Giotino requested a review from sindresorhus May 26, 2020 17:32
@sindresorhus
Copy link
Owner

This looks good except for the missing deprecation warning.

source/core/utils/dns-ip-version.ts Outdated Show resolved Hide resolved
test/http.ts Outdated Show resolved Hide resolved
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
test/http.ts Outdated Show resolved Hide resolved
test/http.ts Outdated Show resolved Hide resolved
test/http.ts Outdated Show resolved Hide resolved
@Giotino Giotino requested a review from sindresorhus May 30, 2020 08:06
@szmarczak
Copy link
Collaborator

https://travis-ci.com/github/sindresorhus/got/jobs/341303159#L480

  Unhandled rejection in test/http.ts

  › ClientRequest.<anonymous> (dist/source/core/index.js:72:256)

  › ClientRequest.origin.emit (node_modules/@szmarczak/http-timer/dist/source/index.js:39:20)

  › emitErrorNT (internal/streams/destroy.js:100:8)

  › emitErrorCloseNT (internal/streams/destroy.js:68:3)

@szmarczak
Copy link
Collaborator

the stack trace is mangled so you have to run the tests without nyc

@Giotino
Copy link
Collaborator Author

Giotino commented Jun 1, 2020

There was a race condition on the two await new Promise (in http and https) used for the tests related to DeprecationWarning.
The promise could have been resolved before the request was resolved, resulting in the destruction of the web server while the request was still running.

@sindresorhus sindresorhus changed the title Type and documentation for family option Add dnsLookupIpVersion option Jun 1, 2020
@sindresorhus sindresorhus merged commit 7f643bb into sindresorhus:master Jun 1, 2020
@sindresorhus
Copy link
Owner

Very nice work, @Giotino 👌

@Giotino Giotino deleted the issue-1263 branch June 1, 2020 17:19
szmarczak added a commit to jaulz/got that referenced this pull request Jul 6, 2020
Co-authored-by: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@szmarczak
Copy link
Collaborator

szmarczak commented Mar 14, 2021

@sindresorhus Maybe let's use numbers but undefined means 'auto'? That way there would be no 0.

@sindresorhus
Copy link
Owner

Yeah, that would be nicer.

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

Successfully merging this pull request may close these issues.

The types are missing family option
3 participants