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

android, unicode urls: Fix correct url not passed to custom tabs. #3317

Closed

Conversation

jainkuniya
Copy link
Member

Before getFullUrl was adding realm even if unicode url string
starts with http. This issue is completely wired. It can not be
reproducible when remote debugger is on, on Android, but can
otherwise.

Replacing startsWith with indexOf doesn't make any logically
change, but have internal implementation difference. Thus this got
fixed.

Before `getFullUrl` was adding `realm` even if unicode url string
starts with `http`. This issue is completely wired. It can not be
reproducible when remote debugger is on, on Android, but can
otherwise.

Replacing `startsWith` with `indexOf` doesn't make any logically
change, but have internal implementation difference. Thus this got
fixed.
Copy link
Member Author

@jainkuniya jainkuniya left a comment

Choose a reason for hiding this comment

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

@gnprice @borisyankov this is also ready to review.

@borisyankov
Copy link
Contributor

Sounds logical that this could be a fix.
Do you happen to have found this issue described or discussed somewhere online?
Would be a good addition to the commit message and would also make it more clear that this is the right fix.

@gnprice
Copy link
Member

gnprice commented Feb 25, 2019

Hmm. Can you say more about exactly what you observed?

The theory in this commit appears to be that String.prototype.startsWith, in the JS environment that RN pulls in, has a bug that can cause it to give a completely wrong answer.

There's a programming cliché that "it's never a compiler bug":
https://blog.plover.com/prog/compiler-error.html
https://blog.codinghorror.com/the-first-rule-of-programming-its-always-your-fault/
The cliché applies equally well to the OS, to the Android platform and iOS SDK... and to the JS standard library. 🙂

Compilers do have bugs! And so do JS standard-library implementations, I'm sure. But these are systems a lot of people have banged on and a lot of people have put good work into making solid -- and so when you're tempted to say it's a compiler bug, or a JS stdlib bug, it's generally much more likely to be your bug, or something you don't understand yet.

And on the rare occasion you do find a JS stdlib bug... you'll want to have it well documented! So the path to resolving the situation well is basically the same either way.

@borisyankov
Copy link
Contributor

borisyankov commented Feb 26, 2019

The most likely place this bug can be is in the polyfill:

Libraries/polyfills/String.prototype.es6.js

@borisyankov
Copy link
Contributor

... or more likely a JSC issue!

Very interesting thread here:
facebook/react-native#11370

Looks like JSC will be upgraded in RN 0.59
facebook/react-native#19737

And that would be a very likely fix for our issue.

@jainkuniya jainkuniya assigned jainkuniya and unassigned jainkuniya May 27, 2019
@jainkuniya
Copy link
Member Author

Let's close this. As I am not able to reproduce the issue. And there is no report for any such or similar issue from users. I am actively testing this and re-open it, if this is still a issue.

Actually the issue was only on Android, startsWith was not working as intendant.

@jainkuniya jainkuniya closed this Jun 15, 2019
@gnprice gnprice added the upstream: RN Issues related to an issue in React Native label Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream: RN Issues related to an issue in React Native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants