-
Notifications
You must be signed in to change notification settings - Fork 55
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
Introduce two new options to enabled/disabled the Private/ICANN domains. #123
Conversation
7e81081
to
df37c59
Compare
Super nice! writing out loud My initial intention with this library was to use PublicSuffix as a database to identify subdomains and make sure TLD were well-known. I think a lot of confusion comes from the fact some functions are here to serve the purpose of PublicSuffix (in a good way), and some are pure utility functions (independant from PublicSuffix). Technically speaking, it's all good. I'd rather give a meaningful name to the options rather than having make users to understand whether things are part of a Public or Private part of a given database. So for example when you write
What do both lines mean from a library perspective? What difference does it make in term of results? |
@@ -12,17 +12,25 @@ var extractTldFromHost = require('./from-host.js'); | |||
* @param {string} hostname | |||
* @return {string} | |||
*/ | |||
module.exports = function getPublicSuffix(rules, hostname) { | |||
module.exports = function getPublicSuffix(rules, hostname, allowIcann, allowPrivate) { |
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.
Here, I'd change allowIcann, allowPrivate
in an options
object.
At the same time, if it's not part of the public API, it does not matter as much.
Thanks for the review! At the beginning I was wondering if we could have a set of options to tweak the behavior of
Maybe there is a way to get the best of both worlds? By having some low-level options as well as high-level ones, which will translate to lower-level options when enabled? Or we could make the distinction between the public API exposed by Do you have suggestions about how this could look like? |
df37c59
to
b10151f
Compare
* 'allowIcann' set to 'false' will ignore the ICANN section of the list. * 'allowPrivate' set to 'false' will ignore the PRIVATE section of the list.
b10151f
to
13ed3e4
Compare
When given as an option to
fromUserSettings
:allowIcann
set tofalse
will ignore the ICANN section of the list.allowPrivate
set tofalse
will ignore the PRIVATE section of the list.This is an attempt to address the recurring issue about non-intuitive results on some domains. In particular, it allows to disable the use of the
PRIVATE
section from public suffix list (You can also disable the use ofICANN
domains but it's probably less useful).Instantiate
tld.js
with the custom option:#120
#117
#111
#25
So most of the issues are solved at this point. There are a few corner cases not handled by this change, but they could be with a few more options to customize the behavior of
tld.js
. In particular, #78 could benefit from an option to disable the use of wildcard rules (such as*.er
which will considergoogle.er
to have public suffixgoogle.er
).@oncletom Do you think it could be useful to also add two fields in the resulting object from
parse
:isIcann
andisPrivate
. This would allow to know what kind of rule was used to parse a given URL/hostname.Also, this PR introduces a breaking change in the way the trie is serialized/represented. Instead of having value
0
as leaf values, it now has either1
(ICANN) or2
(Private).