search

naoufal / react-native-safari-view

A React Native wrapper for Safari View Controller.
https://www.npmjs.com/package/react-native-safari-view
494 stars 110 forks source link

Bug fixes and Perfomance fixes #78

Closed chaithanyaprathyush closed 6 years ago

chaithanyaprathyush commented 7 years ago

Hey, I've been playing around with your awesome libray for a while, and i noticed some issues with it. These are the changes i've made to the code.

96201a3 If the URL is a complex URL with escape characters and horizantal tabs, the method call [NSURL URLWithString: args[@"url"]]; returned a null URL. This commit fixes it. Although i think there are more corner cases that needs to be handled in this case.

bc78a6f I noticed that you were using self.safariView to refer to the internally used SFSafariViewController, Obj-c provided internal variables to be used, which at times helps with performance and memory management

7dfc37f I have a configuration for an application that uses multiple windows and they are not the window provided by the AppDelegate, it is always a better pattern to use [[[UIApplication sharedApplication] keyWindow] rootViewController] instead of the [[[UIApplication sharedApplication] delegate] rootViewController

9b74906 SFSafariViewController is guaranteed to be available after iOS 9 until depreciated, so a version check should be enough.

bb09abe I noticed that you call a delegate method and then dismiss the view controller, instead you could just call dismiss on the view controller which would dismiss it, saving you from a method call and other side effects.

koenpunt commented 7 years ago

In general I think this looks good, so thanks!

But I'm not really happy with the whole string sanitization code; I'm hoping there's another way to achieve that. Do you have an url that demonstrates the problem?

koenpunt commented 7 years ago

And you can remove 7dfc37f, since that has now been replaced by the RN provided helper method RCTPresentedViewController

chaithanyaprathyush commented 6 years ago

@koenpunt can you check if this is fine...

koenpunt commented 6 years ago

I screwed up the branch associated with this PR, so I opened a new one: #85

koenpunt commented 6 years ago

To elaborate, while trying to update the commits, specifically the merge commit which had more in it than just a merge, I accidentally pushed upstream master to your master, and after that I was no longer allowed to force push to that branch. Anyway, I'm giving #85 a final test and if all seems good I'm going to merge. Thanks! 💯

koenpunt commented 6 years ago

While diving into the ivar/@property change, I found that the performance is about nanoseconds, and thus negligible (or at least according to this blogpost: https://www.bignerdranch.com/blog/should-i-use-a-property-or-an-instance-variable/). And also the point about memory management is not totally clear to me. I'm not saying it is wrong, just wondering if it is actually necessary.