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

Update proxy.pac #1

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

Update proxy.pac #1

wants to merge 1 commit into from

Conversation

RobertRosca
Copy link
Member

Use shExpMatch instead of indexOf, allows for wildcard subdomain domain matches

Use shExpMatch instead of indexOf
@RobertRosca RobertRosca requested a review from takluyver October 20, 2021 10:26
@takluyver
Copy link
Member

Does it make sense to do an indexOf check first, and only use shExpMatch where we want wildcards? I did this figuring that the simpler check was probably faster. I'm guessing that the browser has to run this on every URL, so we want to keep it pretty light.

@RobertRosca
Copy link
Member Author

Yeah it does, but it's pretty fast. There are fancier methods you can use to optimise for speed: https://docs.microsoft.com/en-us/troubleshoot/browsers/optimize-pac-performance

So we could do something like:

if not (dnsDomainIs(host, ".desy.de"))
    return "DIRECT";

Which would skip all of the slower checks before getting to them

@takluyver
Copy link
Member

That makes sense, though obviously we'd need to check xfel.eu as well. That Microsoft doc suggests that shExpMatch is faster than dnsDomainIs, but maybe that's specific to Microsoft - this source code I found from Apple suggests that dnsDomainIs is essentially just .endswith(). Either way, being able to return a value for most URLs after just a couple of checks seems like a good idea.

There's one plain IP address in the list, which it looks like @turkot added for OCDM. Is there a domain name for that, or do we have to special case that IP address?

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.

2 participants