-
Notifications
You must be signed in to change notification settings - Fork 576
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
Add libxml2 app link resolver #25
base: main
Are you sure you want to change the base?
Conversation
Needs rebase |
Looks good, but why is the old resolver removed? This will break all old clients. It would be better to have both resolvers and refactor any shared code into a ResolverHelper or similar class. This also adds a dependency to the library - @nlutsenko-fb does that matter? |
I went ahead and made the requested changes – the resolvers now co-exist and share code. I also updated the tests to test both resolvers 👍 Hopefully |
Any progress on this? |
Rebased against master |
@conradev The tests are failing here. |
a8a1dfc
to
4eead63
Compare
@nlutsenko, I rebased against |
The error only appears to be showing in |
Since merging this in will break tests for all PRs - let me try to change our |
🐫 If I rebase against |
I rebased this against master! |
@conradev updated the pull request. |
Rebased again! 👍 |
Whee! Ok, then we'll review it very very soon. |
Rebased again! 😛 |
I went and rebased the PR against master. Would love to see this get merged! ⛵ |
I am new to this friend, please let me know if I shouldn't be doing this: ) Sorry for disruptions Sent from Yahoo Mail on Android On Thu, 30 Jun, 2016 at 23:45, Conrad Kramernotifications@github.com wrote: — |
Am going to review this and get back to you. |
Thank you for your time :) Sent from Yahoo Mail on Android On Fri, 1 Jul, 2016 at 2:00, Nikita Lutsenkonotifications@github.com wrote: — |
I added a
BFXMLAppLinkResolver
to resolve links usinglibxml2
.This is because loading the URL in a
UIWebView
is often unnecessary.