perf: improve tldts.getDomain speed#1936
Conversation
| }; | ||
|
|
||
| export const getDomain = url => getDomain_(url, getDomainSharedOpts); | ||
| export const getPublicSuffix = url => getPublicSuffix_(url, getPublicSuffixSharedOpts); |
There was a problem hiding this comment.
AFAICT the same options can be used for both functions.
There was a problem hiding this comment.
I am worrying about the getPublicSuffix case here:
The input of getPublicSuffix can be invalid IMHO.
There was a problem hiding this comment.
AFAICT it can't be invalid because it's extracted as dot-separated valid sequence via ((?:\.[-\w]+)+)/ or (|(?:\.[-\w]+)+)
There was a problem hiding this comment.
@tophf Exactly! Using regex can not ensure whether the input is a valid domain or not, thus extra check from tldts is required!
Also, the option will affect how tldts handles the invalid TLD:
https://runkit.com/sukkaw/652d2ea9557abc00083e0209
There was a problem hiding this comment.
No, my point is that our regex is guaranteed to extract a valid sequence because it's applied to an actual URL, not to an arbitrary input.
There was a problem hiding this comment.
Also note that optimizing getDomain is pointless performance-wise as we use it only when showing the popup and when the user creates a new script for the tab.
Address suggestions from @remusao (#1883 (comment)).
Update
getDomainSharedOptsforgetDomainas inputs are known to be valid hostnames.The
getPublicSuffixstill uses the old options, asgetPublicSuffixis only used in unit test cases and inputs can be invalid.