8000 perf: improve `tldts.getDomain` speed by SukkaW · Pull Request #1936 · violentmonkey/violentmonkey · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

perf: improve tldts.getDomain speed #1936

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

perf: improve tldts.getDomain speed #1936

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/common/tld.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,19 @@ import { getDomain as getDomain_, getPublicSuffix as getPublicSuffix_ } from 'tl
* tldts does not respect the public suffix list by default, but can be opt in manually
* with the option `allowPrivateDomains`. Hoist the `sharedOpts` can also help avoid
* re-creating the object every time.
*
* Note `extractHostname` and `validateHostname` are set to false because the inputs are
* from `new URL(url).hostname` and are known to be valid
*/
const sharedOpts = { allowPrivateDomains: true };
export const getDomain = url => getDomain_(url, sharedOpts);
export const getPublicSuffix = url => getPublicSuffix_(url, sharedOpts);
const getDomainSharedOpts = {
allowPrivateDomains: true,
extractHostname: false, // inputs are already hostnames
validateHostname: false, // inputs are already valid, no need to perform extra validation
};

const getPublicSuffixSharedOpts = {
allowPrivateDomains: true
};

export const getDomain = url => getDomain_(url, getDomainSharedOpts);
export const getPublicSuffix = url => getPublicSuffix_(url, getPublicSuffixSharedOpts);
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT the same options can be used for both functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am worrying about the getPublicSuffix case here:

image image

The input of getPublicSuffix can be invalid IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

AFAICT it can't be invalid because it's extracted as dot-separated valid sequence via ((?:\.[-\w]+)+)/ or (|(?:\.[-\w]+)+)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

image

Copy link
Member
@tophf tophf Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

0