-
Notifications
You must be signed in to change notification settings - Fork 4
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
Use of node core dns module #14
Conversation
Removed old dns dependency to make the module works only with core module, also removed the event emitters in favor of simple callbacks
Updated README ✏️
Fixed by moving the getResolvers method to separate test, since setServers doesn't play well with async code: see node/issues/1071 this will be fixed in node 8
..I can't exaplain why these tests are working perfectly on my machines (both windows and linux) and not in the travis ubuntu based vm.. wtf......... it's a known issue and work in progress.. so I suppose I'll wait a little bit, but still try to fix this.. one solution is to comment the tests that cause this error or run them in a synchronous way |
This hopefully will fix the multiple calls of setServers which easily bugs
In node v8 this will be resolved using a new Resolver for each validation test, but right now, it's working but sometimes can be buggy.. for example if running multiple async validation requests or multiple scanners. It's not a big deal right now since I assume most of the people will use 1 scan per time, otherwise they will face with this issue. What do you think about it @skepticfx ? can you give a quick review before merging? cheers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! nice set of changes with refactoring.
@@ -6,17 +6,16 @@ var options = { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we name the test folder as tests
or test
instead of __tests__
. Haven't seen this convention used in nodejs projects. Let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets discuss this in later iterations. Merging the pull.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it comes from jest test suite https://facebook.github.io/jest/docs/en/configuration.html#testmatch-array-string, the first time I saw this naming was when I first used yeoman-node generator and from then I decided to stick with this name, but any name (acceptable from jest) it's allright
@skepticfx can you bump the version number to 1.5.0 and publish on npm? as per readme I made a enhanced cli tool for #12 and I need the new changes alive. Also I would like to add an ASCII art that always make stuff cooler but It's glitchy when it renders in console output and I can't find a good one.. any idea? |
i bumped and published 1.4.0. Didn't notice the 1.5.0 that you had. I'll just bump it again to 1.5.0 For the ASCII art, did you try: https://github.com/Marak/asciimo ? |
published to npm subquest@.1.5.0 |
never seen or used asciimo but looks cool, usually a generate one with a random online tool and console log it but I had troubles with indentation with some (most) of them |
This PR had various code changes, documented in README file, to make use of node core dns module instead of native-dns.
This also get rid of event emitters in favor of simpler and more readable callbacks, the tests were updated according to check the new functionality.
Also since there are some structure changes a new version 1.5.0 is proposed.