couchbaselabs / react-native-couchbase-lite

Couchbase Lite binding for React Native Android & iOS
MIT License
111 stars 54 forks source link

Use internal url instead of instantiating the listener on iOS #56

Closed jamesnocentini closed 7 years ago

jamesnocentini commented 8 years ago

The react native plugin instantiates the listener on iOS https://github.com/couchbaselabs/react-native-couchbase-lite/blob/master/ios/ReactCBLite.m#L28. But it should use the internalURL property instead as stated in https://github.com/couchbase/couchbase-lite-ios/issues/1270

In the PhoneGap plugin, the line is https://github.com/couchbaselabs/Couchbase-Lite-PhoneGap-Plugin/blob/master/src/ios/CBLite.m#L35.

npomfret commented 8 years ago

Did this change recently? I don't remember it being a problem until I upgraded to CBL 1.2.1

jamesnocentini commented 8 years ago

No internalURL has been around for a while but the plugin never used it.

npomfret commented 8 years ago

@jamiltz - are you able to work on this? My objective-C skills are pretty poor.

jamesnocentini commented 8 years ago

I'm having a look now. Currently, the plugin allows to pass in a username, password and port via the init method but those parameters won't apply to iOS anymore when using internalURL. I'm trying to think of the best way to handle that. We could remove those parameters altogether from android as well. It would become init(url => { console.log('the url is ' + url);}). For android, the username, password, port would be generated on the java side and passed to the app through the callback above. What do you think?

npomfret commented 8 years ago

Sounds fine. I can do the android bit if you need some help.

npomfret commented 8 years ago

In the event that something goes wrong and the listener cannot be instantiated, the user currently has no way to distinguish between a success callback and an error callback. What do you think to returning with 2 parameters? In java, it'd be something like:

Success: callback.invoke(url, null);

Error: callback.invoke(null, "some error message");

npomfret commented 8 years ago

(If the above is acceptable...), I've committed the change.

The js code to invoke the listener now looks like this:

ReactCBLite.init((url, err) => {
  if(err) {
    console.warn("Failed to start Couchbase lite listener", err);
    return;
  } else {
    // success
    console.log("Couchbase lite listener started at", url);
  }
});

Is there any value in allowing the user to specify username and password? I guess maybe we should support it for existing apps??

Let me know if you need me to change anything.

jamesnocentini commented 8 years ago

Looks good, I think we should have another method called initDev which removes the username/password altogether on Android and for iOS uses the Listener as opposed to interalURL which is only accessible inside the app (not as helpful as a localhost:5984 during development).

npomfret commented 8 years ago

So is this what you have in mind - 2 methods, one that takes a username and password and one that doesn't?

ReactCBLite.init(username, password, (url, err) => {

ReactCBLite.initDev((url, err) => {

I think we need to still have the ability to pass credentials, just for backward compatibility.

jamesnocentini commented 8 years ago

Ok for backward compatibility, I'll make those changes for iOS this afternoon. It might be confusing to pass a username/password for iOS because they will be ignored but I don't think we have the choice.

jamesnocentini commented 8 years ago

I've made the change for iOS on a branch https://github.com/couchbaselabs/react-native-couchbase-lite/tree/56-init-refactor but I'm seeing the following in RN. image I'm not sure why that is. The iOS url is now http://lite.couchbase./.

npomfret commented 8 years ago

Think the url might be correct, this guy has the same one. So maybe that mean the listener isn't started yet?

npomfret commented 8 years ago

@jamiltz - have you made any more progress on this?

jamesnocentini commented 8 years ago

The http://lite.couchbase./ URL isn't working for me. I get the "Network request failed" error in the chrome debugger. I wonder if a newer release of react native may have broken this (I also followed the instructions in the guide you linked above). As a test, https://github.com/couchbaselabs/react-native-couchbase-lite/commit/86225180dfdffbc644cc875fd8aedd2bd8721709 makes the same request in ObjC and it works there. I'm not sure why it's not working from the JS. Can you try branch 56-init-refactor on your machine?

npomfret commented 8 years ago

I'll give it a go. What does that url (http://lite.couchbase./) mean? I don't really understand why it isn't localhost.

jamesnocentini commented 8 years ago

You can have a look at the source https://github.com/couchbase/couchbase-lite-ios/blob/master/Source/API/CBLManager.m#L510. I'm not entirely clear on how it works either. This is the description from the header file:

The base URL of the database manager's REST API. You can access this URL within this process, using NSURLConnection or other APIs that use that (such as XMLHTTPRequest inside a WebView), but it isn't available outside the process. This method is only available if you've linked with the CouchbaseLiteListener framework.

npomfret commented 8 years ago

I've got the same issue.

... but it isn't available outside the process.

Is this the problem I wonder?

yonahforst commented 8 years ago

I was wondering the same, since ReactNative runs on it's own thread in iOS (i think). I just googled the difference between iOS 'thread' and 'process' and found this:

Each process (application) in OS X or iOS is made up of one or more threads, each of which represents a single path of execution through the application's code. Every application starts with a single thread, which runs the application's main function. Applications can spawn additional threads, each of which executes the code of a specific function.

npomfret commented 8 years ago

Perhaps the answer is to restart the listener? These guys are talking about their http server sleeping when the lock screen is on. Their suggestion is to implement applicationWillEnterForeground in AppDelegate. My IOS knowledge isn't good, and I don't know how to get a hold of the listener object in app delegate... or how to implement something like applicationWillEnterForeground in ReactCBLite.m.

npomfret commented 8 years ago

This is a nasty hack, but seems to get around the problem. I tried adding start and stop methods to ReactCBLite.m:

RCT_EXPORT_METHOD(start) {
    NSLog(@"Couchbase Lite starting listener");

    NSError* error;
    if ([listener start:&error]) {
        NSLog(@"Couchbase Lite listening on port <%@>", listener.URL);
    } else {
        NSLog(@"Couchbase Lite couldn't start listener: %@", error.localizedDescription);
    }
}

RCT_EXPORT_METHOD(stop) {
    [listener stop];
    NSLog(@"Couchbase Lite stopped listening");
}

And from my react-native code:

import {AppState} from "react-native";

...

  AppState.addEventListener('change', this._handleAppStateChange);

...

  _handleAppStateChange(currentAppState) {
    if(currentAppState === 'inactive' || currentAppState === 'background') {
      ReactCBLite.stop();
    } else {
      ReactCBLite.start();
    }
  }

Thoughts?

npomfret commented 8 years ago

Any objections to me putting this change into master until we find a more correct solution (maybe see how the cordova plugin archives it)?

I've tested on a device and a simulator but get different results (but I think its ok):

If there are not objections I'll put this into master today. And I'll also see if it can get it to pick a port in the same way that the java implementation does.

npomfret commented 8 years ago

Ok, I've pushed my changes to master. If someone else could run test that would be helpful. You'll need to change your Rn code:

import {AppState} from "react-native";

...

    AppState.addEventListener('change', (currentAppState) => {

      // it prevents the listener from refusing connections when the app goes into the background (in iOS)

      if(currentAppState === 'active') {
        ReactCBLite.startListener();
      } else if (currentAppState === 'inactive') {
        ReactCBLite.stopListener();
      }
    });

I'll update the docs with this after its released.

yonahforst commented 8 years ago

@npomfret - I'm using a similar solution and its working (just calling init again whenever it goes from background to foreground) but I still sometimes get the http error, although way less than before.

I'm still very curious why internalUrl isn't accessible via react native fetch, as that seems to be the ideal solution. I wonder if @snej has any ideas for us.

npomfret commented 8 years ago

I've asked around but haven't had an answer yet. No one really knows enough about both react native and couchbase I think

yonahforst commented 8 years ago

Darn. Do we know anything about how internalUrl actually works? i.e. what the actual underlying iOS mechanism is that serves requests.

I know that the regular listener is based on CocoaHTTPServer, what would be the equivalent for internalUrl? Then we could start investigating as to why it wouldn't be available from react native

npomfret commented 8 years ago

Ugh! Be my guest ;)

For now I'm going to try to get @jamiltz to release the start/stop changes - its a big step forward in terms of reliability. After that, if we can come up with a more correct solution, then great.

snej commented 8 years ago

I'm still very curious why internalUrl isn't accessible via react native fetch

It depends on how React implements the networking for XHRs. If they use NSURLSession, then if they're instantiating their own session instead of using the default one, we'd need to register CBL's NSURLProtocol handler in that session.

If they're not using NSURLSession (or the older NSURLConnection), the internalURL just isn't going to work. But this seems unlikely, unless they're being religiously cross-platform and using something like libcurl...

snej commented 8 years ago

The above seems like something you could research by looking at the React-Native source code. Search for NSURLSession or NSURLConnection. If you don't see those, look at submodules or the podspec or Cartfile to see what external libraries they bring in — anything obviously networking-related?

yonahforst commented 8 years ago

Hey @snej. I was just checking out your comments here and was digging around the source code.

It looks like they ARE using their own session (created with the default config) instead of the shared one

snej commented 8 years ago

Cool. Then all we need to do is somehow splice into it to register our protocol handler with the config used by that session. The code would look like:

    NSURLSessionConfiguration* config = [[NSURLSessionConfiguration defaultSessionConfiguration] copy];
    // Register Couchbase Lite's NSURLProtocol. This allows CBL to handle HTTP requests made by this
    // session that target the internalURL of a CBLManager.
    Class cblURLProtocol = NSClassFromString(@"CBL_URLProtocol");
    if (cblURLProtocol)
        config.protocolClasses = @[cblURLProtocol];

This would probably require talking with the React developers to work out a way for us to hook into their session creation, since I assume they wouldn't want to add the above code directly to their codebase. I'm happy to be involved in that.

(There are probably ways to get the above to happen even without any hooks in React, but they'd involve using the Obj-C runtime API to splice code into their classes, which would be a kludge.)

snej commented 8 years ago

I've just asked on the macnetworkprog mailing list whether there's a way to modify the default NSURLSessionConfiguration. If we could do that, we wouldn't need to change React at all.

yonahforst commented 8 years ago

That's awesome! I've been looking to see if there are any react native http libraries which bypass fetch and use [NSURLSession sharedSesson] directly. There are some networking libraries but it look like they've all changed their implementation to use fetch.

I'll try to start a discussion with the react native folks about hooking into session creation and link to it here.

yonahforst commented 8 years ago

I've created a product pain here: https://productpains.com/post/react-native/ios-networking-register-custom-nsurlprotocols

@jamiltz @npomfret - are there any other places where I can post about this to the RN folks?

yonahforst commented 8 years ago

@snej I've monkey patched react native with your code snippet above, but I still can't seem to access the internalUrl from react native.

snej commented 8 years ago

Hm. Unfortunately there's no logging in CBL_URLProtocol, but you can try setting breakpoints at

Or alternatively you could add Log(@"...") calls to those methods and look for the log output at runtime.

npomfret commented 8 years ago

@yonahforst did you make any progress with this?

yonahforst commented 8 years ago

Nope. No luck yet. I couldn't try any of the breakpoints @snej recommended because I'm using a compiled version of cbl and don't have access to the .m files

snej commented 8 years ago

You don't need access to the source. Just create a symbolic breakpoint in Xcode — go to the breakpoint navigator, press the "+" button at the bottom, choose "Add Symbolic", and enter the method name exactly as I gave it above.

yonahforst commented 8 years ago

Oh, I didn't know you could do that! Cool, I'll try it tomorrow and post the results

yonahforst commented 8 years ago

Ok, I set the breakpoints and canInitWithRequest is getting called for every RN request but handlesURL is returning NO for http://lite.couchbase./

yonahforst commented 8 years ago

OK, I got it! I wasn't calling [dbmgr internalURL] (Since I was hardcoding it as string in RN). Re-reading @snej's comments above, I realize that I need to call it to register the server with CBL_URLProtocol

I can now instantiate my DB using the internalURL! 🎉🚀

import {manager, ReactCBLite} from 'react-native-couchbase-lite'
ReactCBLite.init((url) => {
      var database = new manager(`http://lite.couchbase./`, 'myapp');
      database.createDatabase()
        .then(() => database.getAllDocuments())
    });
Quick summary of steps to get it working:

1 - In Xcode, open Libraries > RCTNetwork.xcodeproj > RCTHTTPRequestHandler.m 2 - make the following change to - sendRequest:withDelegate:delegate:

    //...
    NSURLSessionConfiguration *configuration = [NSURLSessionConfiguration defaultSessionConfiguration];

    // Register Couchbase Lite's NSURLProtocol. This allows CBL to handle HTTP requests made by this
    // session that target the internalURL of a CBLManager.
    Class cblURLProtocol = NSClassFromString(@"CBL_URLProtocol");
    if (cblURLProtocol)
      configuration.protocolClasses = @[cblURLProtocol];

    _session = [NSURLSession sessionWithConfiguration:configuration
                                             delegate:self
                                        delegateQueue:callbackQueue];
    //...

3 - Still in Xcode, open Libraries > ReactCBLite.xcodeproj > ReactCBLite.m 4 - make the following change to - init:username:password:callback::

    //...
    [listener start:nil];

    NSURL *internalUrl = [dbmgr internalURL];
    NSLog(@"Couchbase Lite url = %@", internalUrl);
    callback(@[[NSNull null], [internalUrl absoluteString]]);
    //...

5 - Done. Change your manager in JS to instantiate with http://lite.couchbase./ Note: I did start seeing an error in the console WARNING: Exception caught in CBL_Router: data parameter is nil but it seems to be addressed here: https://github.com/couchbase/couchbase-lite-ios/issues/1045

Followup

@snej Thanks so much for your help on this. I saw Quinn "The Eskimo!" replied to your question about modifying the default NSURLSessionConfiguration saying that it's not possible. So if we want to use this solution, we'll need to make a case for hooking into the session creation without our custom protocols.

npomfret commented 8 years ago

Amazing work, this is a big step forward - thank you both for getting here! So what's next? Is there a way to do this without modifying react-native code?

snej commented 8 years ago

Awesome!!

It may be possible to patch this in without modifying ReactNative, but only through ugly hacks like using the Obj-C runtime to splice new code into it. It would be far better to work with the ReactNative developers to get some sort of hook added to it that you can plug into. (See earlier comment.)

yonahforst commented 8 years ago

My guess is that this would be a low priority for the RN devs. We would probably have better luck submitting a PR. Anyone up for the challenge?

Actually, browsing through through the RN code, it looks like they already have a mechanism in place for registering custom request handlers by implementing RCTURLRequestHandler https://github.com/facebook/react-native/blob/master/Libraries/Network/RCTNetworking.m#L146-L200

Although I'm not sure how to actually register our handler. It might be as simple as calling RCT_EXPORT_MODULE() I'm gonna give it a shot, using this as an example: https://github.com/facebook/react-native/blob/master/Libraries/Network/RCTDataRequestHandler.m

npomfret commented 8 years ago

Maybe just ask the question on their github ?

yonahforst commented 8 years ago

Well, they are pretty adamant about using github for bugs only and I don't want to piss them off 😅

In any case, it worked! I basically copied our monkey patched code into it's own class and set a handlerPriority so it gets chosen over the default handler. RCT_EXPORT_MODULE() registered the module as a handler and that was it!

Check it out on my fork: https://github.com/yonahforst/react-native-couchbase-lite/blob/master/ios/ReactCBLiteRequestHandler.m

Two things I'm not sure about: 1 - in the canHandleRequest: method, I'm just checking for the ".couchbase." suffix. I'd like to pass this check off to CBL_URLProtocol but it's private. Should we try to access it anyway? or is checking the suffix enough. 2 - There's a lot of code copied over from the default implementation. I don't understand most of it but I'm thinking we might be able to trim it down? Or would subclassing the default implementation be a better solution? What do you guys think

npomfret commented 8 years ago

I'm not really sure, but I'm more concerned with getting something working. Yes, it'd be great to have a clean solution. But for now, why don't we just added it to the project and get testing. We can alway clean up later. If it works, it works!

npomfret commented 8 years ago

Just tried this out on my fork and it works well. It even seems to work when my app receives a push notification and is effectively running in the background - amazing!

Can I suggest we add these 2 files in? We don't need to make any other code changes as the old style listener works fine. The user now has the option to use the internal url or the old listener . I'd just add something to the documentation explaining that the option exists, maybe something like:

  _onCBLiteInit(cblUrl, err) {
    console.log("couchbase lite started at", cblUrl);

    if(Platform.OS === 'ios') {
      cblUrl = "http://lite.couchbase./";
      console.log("Using couchbase lite internal url", cblUrl);
    }
yonahforst commented 8 years ago
snej commented 8 years ago
  1. Not sure, but it's a great question. Why don't you ask on the Google group or the Couchbase web forum?
  2. No; Android doesn't have an equivalent of the NSURLProtocol hook that allows apps to handle URL schemes in-process.