dns: Use object without protoype for map #5843
Conversation
benjamingr
added
dns
Currently we use `{}` for the `lookup` function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:
```js
dns.resolve("google.com", "toString", console.log);
```
And get `[Object undefined]` logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to `Object.prototype` might
also create unexpected results.
This pull request fixes it, with it an appropriate error is thrown.
|
Please try to remove the qs readme rename from your git history. :S |
|
Seems fine to me, since we only ever make it one we don't need to worry about the perf hit. |
benjamingr
force-pushed
the
dns-refactor-object-map-to-createnull
branch
from
ef50aeb
to
6f584af
Compare
|
@Fishrock123 do you have any idea how that could happen (the qs readme rename)? (Fixed.) Also, why would there be a perf hit in these cases - is What do you think about the semver? |
git with case-sensitive filesystems. At one point there was a move and rename as the same time with the same name and different casing. Turns out, git usually doesn't like that and you end up with problems.
It is something like 70 times slower to create. So, possibly an issue if you do it a lot. probably semver-patch |
benjamingr
removed
the
semver-major
Oh! That makes sense, thanks!
Ok, good to know.
Thanks, and thanks for the review. |
|
Seems reasonable. May want to include a test case. |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/2022/console Added test, good idea. |
|
LGTM if CI is green |
|
LGTM |
1 similar comment
|
LGTM |
Currently we use `{}` for the `lookup` function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:
```js
dns.resolve("google.com", "toString", console.log);
```
And get `[Object undefined]` logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to `Object.prototype` might
also create unexpected results.
This pull request fixes it, with it an appropriate error is thrown.
PR-URL: #5843
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
Landed in c9c387f |
|
Thanks. Aren't we supposed to wait 48h on these? (Newbie question, I've only been added as a collaborator a week ago) |
|
Generally, yes, this one is quite simple, had green CI, and got plenty of LGTM's pretty quickly, seemed worthwhile. |
|
Great, thanks. |
Currently we use `{}` for the `lookup` function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:
```js
dns.resolve("google.com", "toString", console.log);
```
And get `[Object undefined]` logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to `Object.prototype` might
also create unexpected results.
This pull request fixes it, with it an appropriate error is thrown.
PR-URL: #5843
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Currently we use `{}` for the `lookup` function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:
```js
dns.resolve("google.com", "toString", console.log);
```
And get `[Object undefined]` logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to `Object.prototype` might
also create unexpected results.
This pull request fixes it, with it an appropriate error is thrown.
PR-URL: #5843
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Currently we use `{}` for the `lookup` function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:
```js
dns.resolve("google.com", "toString", console.log);
```
And get `[Object undefined]` logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to `Object.prototype` might
also create unexpected results.
This pull request fixes it, with it an appropriate error is thrown.
PR-URL: #5843
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Pull Request check-list
Affected core subsystem(s)
dns
Description of change
Please provide a description of the change here.
Currently we use
{}for thelookupfunction to find the relevantresolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:
And get
[Object undefined]logged and the callback would never becalled. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to
Object.prototypemightalso create unexpected results.
I recalled a client actually ran into this issue in production a while ago so now that I'm touching a bunch of dns I figured I might as well fix it :)
This pull request fixes it, with it an appropriate error is thrown.
I tagged this semver-major to be on the safe side since an error is thrown where strange behavior currently happens. This is more likely a bug fix but I figured I'd error on the safe side - this is more like a bug fix.
Please advise.