-
Notifications
You must be signed in to change notification settings - Fork 7.3k
net: Allow custom DNS resolvers #8475
Comments
I like it. It'll allow to more easily tool things like creating a host lookup cache to reduce dns resolution time. |
net.connect now checks whether the user has provided a dns resolution function, this arguments and expected output for which mirror the existing dns.lookup implementation. Fixes nodejs#8475
I thought I'd have a go at doing this as a first time contributor. I basically gave the user the option to substitute a function for dns.lookup, which is what is current used by net.connect. I also added some documentation and a test. Hopefully I'm on the right track :) |
What is it exactly we're trying to solve for here? Keep in mind that we have two kinds of name to ip resolution models in Node.js. First we have The contract of the interface for The second interface of
So what are we trying to solve for? What use case are we trying to satisfy? A generic way to override the IP for a specific request? I could use my "custom" resolver before calling Override the way all future Are you trying to avoid having
I'm not going to say this isn't a necessarily an easy change for someone to make, but before we make a change we need to make sure we understand what we're trying to solve for, and who our consumers will be. Food for thought. |
The idea of the change is to prevent us from needing to think of/maintain the "appropriate" way to handle this issue. Create an appropriate API to allow users to figure out the best solution for their needs. |
I'm a bit lost as to purpose, too. Users can resolve the name any way they think best, and then pass the IP to net.connect, instead of passing a function to net.connect which it then calls in order to do the resolution you didn't. |
I can't find it anymore but there was an issue around the same time where someone asked for a way to time the DNS queries that Yes, the user can separate the query from the connection - but as net.connect() already conflates the two, you might as well make it pluggable. Done right, it should also clean up the connection logic in lib/net.js. |
As @bnoordhuis explained, that is my hope for introducing these types of hooks. |
@chrisdickinson @trevnorris @bnoordhuis ... any further thoughts on this one? |
It's been added to io.js. It should be possible to back-port to joyent/node if someone wants to take that on. If not, I suggest closing. |
I'll mark it as iojs-backport and defer but will also close it. I'm happy just to pick it up downstream |
Per @bnoordhuis's suggestion in #2868:
The PR should be against master -- this would be a good first feature for someone looking to contribute (be sure to include tests, doc updates, and to follow the guidelines for contributing).
The text was updated successfully, but these errors were encountered: