Skip to content
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

isFQDN option require_tld also supports an array to support validating against known values #1248

Closed

Conversation

AubreyHewes
Copy link

@AubreyHewes AubreyHewes commented Feb 11, 2020

This just adds the possibility of validating against a list of given TLD values...

The require_tld option can now be a boolean (backwards compatibility) or an array of TLDs which first applies require_tld: true logic and then that the possible TLD is in the passed array.

  • localhost IS NOT require_tld: true
  • localhost.tld IS require_tld: true
  • localhost IS NOT require_tld: ["tld"]
  • localhost.tld IS require_tld: ["tld"]
  • localhost.example IS NOT require_tld: ["tld"]

This gives more options to the developer, for example, checking the TLD against the IANA gTLD list

Checking against the list itself SHOULD NOT be part of the library.

solves #1193 #1247 #704 #1180

🤗

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your 1st contrib! 🎉 This LGTM.

Let me hear what the rest think on the comments you've made in the listed issues, then we can land this.

@AubreyHewes
Copy link
Author

A possible other solution is to introduce an extra option.. (so keeping the interface stricter) though as this method is used by upstream methods this would also require adding the extra option (or option possibility) to the upstream methods. This would entail a much larger change

@profnandaa
Copy link
Member

/cc. @tux-tn @ezkemboi

@@ -30,6 +30,10 @@ export default function isFQDN(str, options) {
if (/[\s\u2002-\u200B\u202F\u205F\u3000\uFEFF\uDB40\uDC20]/.test(tld)) {
return false;
}
if (Array.isArray(options.require_tld)) {
// TODO Array.includes though for browser BC does this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very curious about this comment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezkemboi yeah I am as well, comment should be removed.. and includes should be used (next line).. though then IE is no longer supported.

@tux-tn
Copy link
Member

tux-tn commented Feb 20, 2020

Thank you @AubreyHewes for your first PR 🎉 !

I really like the idea of handling multiple TLDs from user side and having this feature backward compatible but i have two issues related to api/lib consistency:

  • Option name is misleading, all require_x options elsewhere are booleans. Having it accept an array only for this one bothers me as a user.
  • The option behavior change will also affect isURL and isEmail validators

Also, i guess the readme entry and transpilled version for the change are missing in the PR.

Generally, I agree more with the second solution of introducing a new option for handling this.

@profnandaa
Copy link
Member

Ping @AubreyHewes

@AubreyHewes
Copy link
Author

@profnandaa Sorry was indisposed..

So what do you want? It seems, a second option that iterates through all usages, so this would require a second option to all usages of the isFQDN function as well as itself..?

The option behaviour change actually reduces the amount of required code. Reducing the library footprint.

We could typescript it as either a boolean or an array then it would be explicit for a user.

@profnandaa
Copy link
Member

@AubreyHewes -- sorry about that, good to see you back.

@subalee
Copy link

subalee commented May 6, 2020

@AubreyHewes not sure typescript typescript declaration would work unless you use typescript or vscode?

What i'd like to see is either have a require_official_tld option that would compare the tld against all official list of tld. Other option would be to use something like valid_tlds which would be an array of valid tlds and basically offload the library from having to keep tabs on valid tlds.

my 2 cents.

@AubreyHewes
Copy link
Author

AubreyHewes commented May 8, 2020

@subalee see #1248 (comment)

To summarize; a new option would require a new option to all current and future usages of the isFQDN function as well as itself.. increasing the library footprint. Libraries like this one should always try to be as small as possible in code and as descriptive as possible in the interface documentation. (IMO).

my 2cts ;-) good luck with your first PR #1297 as that does what the feedback was for this.. When I asked if that is what was wanted I never got an answer.

(I do not completely agree with #1297 but, hey that is what forks are for! 👍 )

@AubreyHewes AubreyHewes closed this May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants