Make rules with no exceptions valid #52
Conversation
| @@ -141,7 +141,7 @@ tld.prototype.getRulesForTld = function getRulesForTld (tld, default_rule) { | |||
| } | |||
|
|
|||
| // Nothing found, apply some default value | |||
| if (!rules) { | |||
| if (rules === void 0) { | |||
There was a problem hiding this comment.
To properly reflect the intent, we should have then if (!rules || rules.length === 0) { right?
I am afraid === void 0 has no real signification for a user.
There was a problem hiding this comment.
Rules having a length of 0 is a valid case.
Example: tldjs.rules.eu.length => 0
"Nothing found" would mean the tld is undefined, so the check against void 0 should be the correct one.
Example: tldjs.rules.imagination === void 0 => true
There was a problem hiding this comment.
I am not sure to get it right, as your test relates to .eu, and for this same case, the rules === void 0 syntax does not tell me what to expect in that case (going into the if statement or not).
To be fair, I do not see the difference between rules === void 0 and !rules (which is a shortcut for rules === null || rules === undefined).
There was a problem hiding this comment.
!rules captures '', null, undefined, 0, false - any falsey value.
Unfortunately, undefined is writable, so not completely safe, whereas void 0 ensures the undefined value is used. See http://stackoverflow.com/questions/4806286/difference-between-void-0-and-undefined.
Checking against undefined/void explicitly is the proper way to handle this, rather than a catch-all for a falsey value, then making sure it's not an empty string.
There was a problem hiding this comment.
OK gotcha :-)
This project code is enforced in strict mode so undefined cannot be rewritten. In addition, if rules === false, we do consider we have no rules, so we want to go into that if statement.
A more accurate condition in that case would be if (Array.isArray(rules)) as ES5 is our minimal requirement.
There was a problem hiding this comment.
As far as I can tell, rules is a string (based on looking at https://github.com/oncletom/tld.js/blob/f1477fbae4d60c1c2eb3b5dfc91e969dd7eb00a0/rules.json). It can be "", "*", or a longer string that is | delimited.
If it's undefined, it means that the requested TLD does not exist in rules.json, thus is not a known TLD, so it should have a default rule applied.
In the existing code, trying to find a public suffix on any TLD that is defined to have a rule of "" will fail because the default rule is applied. But something like .eu is a valid domain, so we don't want default rules applied.
Or am I misunderstanding, and "" means we should apply a default rule? I'm currently reading it as "apply the default rule only if the TLD is unknown"
There was a problem hiding this comment.
You are right @GreyKn, it seems that I am not familiar anymore with my own codebase ;-)
I now understand why the if statement check is problematic, sorry if it has been a pain to explain!
Make rules with no exceptions valid
|
Thanks @GreyKn and @andreasheim, it's now on npm as of |
Fixes a problem where suffixes that are valid (like .fm and .eu) but were falling to parse correctly: