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

Using indexOf() instead of Regexp() #122

Closed
wants to merge 6 commits into from
Closed

Using indexOf() instead of Regexp() #122

wants to merge 6 commits into from

Conversation

gorhill
Copy link
Contributor

@gorhill gorhill commented Oct 25, 2013

Performance improvement seen at (along with the intermediate experiments using binary search):
http://jsperf.com/uri-js-sld-regex-vs-binary-search/4

Memory footprint on my 32-bit OS:
Regexp: > 500K
indexOf: < 20K

A test has been added to ensure all SLD are correctly processed by the new code.

@gorhill
Copy link
Contributor Author

gorhill commented Apr 13, 2014

Moving on.

@gorhill gorhill closed this Apr 13, 2014
@rodneyrehm
Copy link
Member

Why did you close the PR? I've only not merged this PR because I wasn't sure if I'd want to go with 1.13 or 2.0 and fix some other things as well. Meanwhile it's clear that 1.13 will have to come first, as 2.0 is a longer way out. Please reopen the PR.

@gorhill
Copy link
Contributor Author

gorhill commented Apr 19, 2014

I was wondering if you would be interested in the ability to support the full Public Suffix List? I think it's not too much work to fit in there publicsuffixlist.js.

User could optionally supply his own list, while the default (stock) list (if no external list supplied) would be a match of what SecondLevelDomain currently handles.

@rodneyrehm
Copy link
Member

I would prefer using PublicSuffix as it used by browsers and maintained properly. I haven't used PS because it also contains entries like github.io, which are not SLDs, but used by the browser for cookie origin control.

If your list filters these out, we can talk about depending on or integrating your implementation (or a derivation thereof). If it doesn't, don't see how this could work.

btw. this should probably be a separate issue. care to open one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants