-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
40fc330
to
5948037
Compare
e81b89f
to
805dc4f
Compare
4e3316f
to
2ec8b2a
Compare
@alanshaw can you review this one when you can? |
1d87bb0
to
19e001a
Compare
19e001a
to
5e02108
Compare
Thanks @niinpatel - I will look at this as soon as I get a moment! |
@alanshaw did you get a chance to check it out? |
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.
Awesome work @niinpatel 🚀
src/core/runtime/dns-nodejs.js
Outdated
module.exports = (domain, opts, callback) => { | ||
resolveDnslink(domain) | ||
// recursive is true by default, it's set to false only if explicitly passed as argument in opts | ||
const recursive = opts.recursive === undefined || opts.recursive.toString() === 'true' |
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.
Default true for null
/undefined
and boolean value of argument otherwise:
const recursive = opts.recursive === undefined || opts.recursive.toString() === 'true' | |
const recursive = opts.recursive == null ? true : Boolean(opts.recursive) |
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.
Boolean("false")
actually returns true
not false
. Arguments passed via query parameters are of string
type and we need to parse those too. Once easy solution I just now came up with is this:
const recursive = opts.recursive.toString() !== 'false'
If recursive is false
or "false"
then only we parse it as false. true
when passed anything else, including null
and undefined
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.
Edit: That wouldn't work either because error will be thrown if opts.recursive is undefined.
The original one is probably the best solution after all, with slight modifications:
js-ipfs/src/core/runtime/dns-nodejs.js
Line 12 in d6b2ca3
const recursive = opts.recursive == null || opts.recursive.toString() !== 'false' |
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.
Arguments passed via query parameters are of string type and we need to parse those too
It should be passed as a boolean - this is the documented type for the value. If a string is being passed then we need to fix our HTTP endpoint to properly parse query strings to the expected types for the calls to core.
We can't guarantee the passed value is a boolean but we can ensure that the value is a boolean for the rest of our code.
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.
That makes sense. Right now, I don't think query parameters are being parsed and "type-casted" before passing to the core API. For this PR, I've just now add it here:
js-ipfs/src/http/api/resources/dns.js
Lines 14 to 18 in fb114f1
// query parameters are passed as strings and need to be parsed to expected type | |
let recursive = request.query.recursive || request.query.r | |
recursive = !(recursive && recursive === 'false') | |
const path = await request.server.app.ipfs.dns(domain, { recursive, format }) |
d6b2ca3
to
8824c91
Compare
8824c91
to
d5fa74e
Compare
fb04535
to
fb114f1
Compare
I've applied all the suggested changes. |
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.
LGTM, thank you @niinpatel ❤️
fixes #1931 .
Also, incorporated the suggestions from https://github.com/ipfs/interface-js-ipfs-core/issues/377 that
ipfs dns
should be recursive by default.