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.

Sign up for GitHub

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

Jump to bottom

Instantiate CacheableLookup only when needed #1529

Merged
merged 3 commits into from
Nov 19, 2020
Merged

Instantiate CacheableLookup only when needed #1529

merged 3 commits into from
Nov 19, 2020

Conversation

fungiboletus
Copy link
Contributor

@fungiboletus fungiboletus commented Nov 19, 2020 •
edited
Loading

The dnsCache option is disabled by default but a CacheableLookup instance is always created. The proposed change is to instantiate CacheableLookup only when needed, which may make got a bit faster to load by default and when you do not use the DNS cache.

Moreover, the CacheableLookup class instantiates the DNS Resolver class in NodeJS the standard library, and the test framework Jest has a problem with it. It wrongly detects that something keeps running after the tests are finished, while it's just an instance of the DNS Resolver class. See jestjs/jest#6423

Jest should be fixed but meanwhile this small improvement in got may also hide the Jest issue for many developers.

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates in both the README and the types.

@fungiboletus
Instantiate CacheableLookup only when needed
f5fb0cf 
The `dnsCache` option is disabled by default but a `CacheableLookup` instance is always created. The proposed change is to instantiate CacheableLookup only when needed, which may make `got` a bit faster to load by default and when you do not use the DNS cache.

Moreover, the CacheableLookup class instantiates the DNS [`Resolver` class in NodeJS the standard library](https://nodejs.org/api/dns.html#dns_class_dnspromises_resolver), and the test framework [Jest](https://github.com/facebook/jest/) has a problem with it. It wrongly detects that something keeps running after the tests are finished, while it's just an instance of the DNS Resolver class. See jestjs/jest#6423

Jest should be fixed but meanwhile this small improvement in got may also hide the Jest issue for many developers.
@fungiboletus
Explicit type of the globalDnsCache variable.
e7e2077
@fungiboletus
Fix code style
b4440ed
@fungiboletus
Copy link
Contributor Author

It looks like the tests succeed excepts with Node 12. But I'm not sure it's related to the changes in this pull request. The last commit on the master branch was also failing on Node 12 : https://github.com/sindresorhus/got/runs/1342216844

@szmarczak
Copy link
Collaborator

This will cause the CacheableLookup instance to be genereated on every got call, which we don't want. dnsCache: true needs to point to a global instance, which is the current code. Unfortunately the bug needs to be fixed by Jest themselves.

@szmarczak szmarczak closed this Nov 19, 2020
@fungiboletus
Copy link
Contributor Author

fungiboletus commented Nov 19, 2020 •
edited
Loading

Are you sure ? I think it will be generated only the first time. It's still a global instance, just not initialized before it's used.

@szmarczak
Copy link
Collaborator

It's still a global instance, just not initialized before it's used.

043c950

@szmarczak
Copy link
Collaborator

I'm sure.

@fungiboletus
Copy link
Contributor Author

I understand you don't want to create one instance for each call, and this test is still successful with my suggested modification.

https://travis-ci.com/github/sindresorhus/got/jobs/443599127

image

@szmarczak
Copy link
Collaborator

Ah, I see now. I didn't notice the if.

@szmarczak szmarczak reopened this Nov 19, 2020
@szmarczak szmarczak merged commit 5fefc20 into sindresorhus:master Nov 19, 2020
@fungiboletus
Copy link
Contributor Author

No problems, I understand why you were suspicious. Many thanks for the merge!

@fungiboletus fungiboletus deleted the cacheablelookup-delayed-instantiation branch November 19, 2020 18:31
@szmarczak
Copy link
Collaborator

Thanks to you for the PR! Never underestimate your work :)

jeremysf added a commit to propellerfactory/node-openid-client that referenced this pull request Dec 30, 2020
@jeremysf
Pulling in sindresorhus/got#1529
2bdad4e
jeremysf added a commit to propellerfactory/node-openid-client that referenced this pull request Dec 30, 2020
@jeremysf
Pulling in sindresorhus/got#1529
1da340a
@yinzara
Copy link

yinzara commented Feb 19, 2021 •
edited
Loading

Could we get a release cut of got (I assume 10.8.2) to get this and other issues fixed since the last release? It's been 4 months.

@szmarczak
Copy link
Collaborator

No, there is v11 already and v12 is pending.

sindresorhus pushed a commit that referenced this pull request Feb 26, 2021
@fungiboletus @sindresorhus
Instantiate CacheableLookup only when needed (#1529)
3bd245f
@sindresorhus
Copy link
Owner

We decided to back port it: https://github.com/sindresorhus/got/releases/tag/v11.8.2

This was referenced Mar 13, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Giotino Giotino Giotino approved these changes

@sindresorhus sindresorhus Awaiting requested review from sindresorhus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fungiboletus @szmarczak @yinzara @sindresorhus @Giotino