Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

Replace HTTP client with SwaggerJS #93

Merged
merged 11 commits into from
Dec 21, 2016
Merged

Replace HTTP client with SwaggerJS #93

merged 11 commits into from
Dec 21, 2016

Conversation

jamesnocentini
Copy link
Contributor

@jamesnocentini jamesnocentini commented Oct 17, 2016

FOR REVIEW ONLY - DO NOT MERGE (WIP)

Example project instructions: https://github.com/couchbaselabs/react-native-couchbase-lite/tree/feature/swagger/ReactNativeCouchbaseLiteExample#getting-started-tested-on-ios-only

Todos:

  • Fix code to show thumbnails (probably loading them as base64 in an Image component)
  • Don't restart the listener every time
  • Write documentation

Optional:

  • Re-add bridging methods (manually specifying the username/password, saveAttachment, getAttachment etc.)

@jamesnocentini jamesnocentini force-pushed the feature/swagger branch 2 times, most recently from 1566ff5 to 77e60bc Compare November 28, 2016 14:55
@jamesnocentini
Copy link
Contributor Author

@npomfret Can you try to run the sample project on android and ios to see if it works on your setup?There's a link to the instructions to the sample project readme above.

@jamesnocentini
Copy link
Contributor Author

This is what the usage & documentation section of the README would then look like https://github.com/couchbaselabs/react-native-couchbase-lite/tree/feature/swagger#usage

It's much easier to maintain but we need to make sure it doesn't make it harder.

- add swagger-client and spec
- update readme
- update .gitignore
- update ReactNativeCouchbaseLiteExample for iOS and Android
@npomfret
Copy link
Contributor

I'm getting 401s back. I can't see where the auth stuff is done?

(Also, I made a couple of minor changes - hope that's ok)

@jamesnocentini
Copy link
Contributor Author

Thanks for the additional commits! About the auth stuff, the username/password gets passed to the manager on this line. However, as you pointed out it still returns a 401 Unauthorized on iOS. I think the fix is to pass the credentials as a header as we currently do. I'll try this out.

@jamesnocentini
Copy link
Contributor Author

For some reason auth works fine when I tested it just now. Did you run it on simulator or device? What iOS version?

- Bundle pre-built db on iOS/Android
- Rename Movie component to Task
- Rename ListMovies component to List
- Call setupQuery once the map/reduce view is created
- Add installPrebuiltDatabase(String name) API method to iOS/Android
@jamesnocentini
Copy link
Contributor Author

jamesnocentini commented Nov 30, 2016

@npomfret I updated the example proj and readme. Added a method to use a pre-built database in the process.

With regards to the API, the index.js module exposes two objects:

  • ReactCBLite: to access bridging methods like init, initWithAuth etc.
  • rncblite: to access the manager.

I think we could combine those into one. Then it would look like:

const Couchbase = require('react-native-couchbase-lite');

// Native module method
Couchbase.installPrebuiltDatabase("todo");

// HTTP client
Couchbase.initRESTClient(manager => {
    // Send HTTP requests...
    manager.get_all_dbs()
      .then(res => {
        // Process response
      });
});

@npomfret
Copy link
Contributor

Can't get the demo running on iOS still:

screen shot 2016-12-19 at 17 25 24

@jamesnocentini
Copy link
Contributor Author

Weird, I don't understand what this error is about. Did you follow the steps in https://github.com/couchbaselabs/react-native-couchbase-lite/tree/feature/swagger/ReactNativeCouchbaseLiteExample#getting-started?

Oh yes I overwrote your changes by mistake when copying my changes from the example project to index.js. Will revert back to your changes tomorrow.

@npomfret
Copy link
Contributor

Don't worry about my changes, I can do them again easily enough.

Can't see what I've done wrong but will give it another go.

@jamesnocentini
Copy link
Contributor Author

Ok thanks. It'd be great if you could try on android too.

@npomfret
Copy link
Contributor

ok, got it working on iOS. Android is failing, getting a 401 from the _all_dbs endpoint. I'm a bit stuck, got no idea how to debug it.

@jamesnocentini
Copy link
Contributor Author

jamesnocentini commented Dec 20, 2016

I see the same error on Android, we have to pass the credentials in the Authorization header instead of inlined (like we do currently). Let me take a look.

Replacing this line with Couchbase.initWithAuth("", "", url => { works as expected, on the other hand.

@jamesnocentini
Copy link
Contributor Author

Can you try running it on android again? You'll have to run npm install ./.. to reload the module in the example project.

I haven't figured out why a URL of type user:pass@host doesn't work but having the credentials base64 encoded in the Authorization header does work. Might be something to do with RN. Unfortunately, as a result, images won't show in the ListView because the credentials for that are passed in the image URL.

@jamesnocentini
Copy link
Contributor Author

jamesnocentini commented Dec 20, 2016

Enhancement idea: it would be nice to check in the init method if there is already a listener running so we don't restart a listener on every reload.

@jamesnocentini
Copy link
Contributor Author

Arf, I don't think it's that easy to determine if the listener is already running because the ObjC/Java also gets reloaded (I was hoping to have a isListenerRunning instance variable there). So I'll give up on that enhancement idea. The last task is to update the documentation then, I'll work on it tomorrow. Can you think of anything else before we merge this?

@npomfret
Copy link
Contributor

npomfret commented Dec 21, 2016

ok, cool. it's working on android, but without images as you mentioned. Once this facebook/react-native#7791 (comment) get's added to RN they should start working. In the mean time, I added a work around into the android API. If you use null/null for the credentials it turns off authentication and also binds the listener to localhost only as a security precaution to prevent off-device access (so I think it works in a similar way to the iOS native listener).

Can we merge it? Here's a brain dump of things we could add - sorry its a bit unstructured. And not sure if any of this would prevent a release.

  • the code is confusing - it's half a movies app and half a fruits app... what's it supposed to do? (fixed in 4f76c8f)
  • get images working in android using the null auth method (maybe?) (move to Images not showing in sample app #102)
  • use the internal iOS listener as it behaves much better than the http listener (which I recall just stops working as soon as the app is backgrounded), we could show how to switch off the other listener (move to Use the internal listener on iOS by default #103)
  • we should show off some features: (move to Enhance sample app #104)
    • pull and push replication (live update the app as data comes in to the changes feed)
    • update documents
    • add documents
    • delete documents
    • purge documents
    • find docs using views
    • (batch) load docs by id
    • add attachments
  • and some performance measurements - it'd be cool to just dump out how long each http request takes (i'm particularly interested in this... been shocked by how slow some queries seem to be) (move to Performance measurements #105)

@jamesnocentini jamesnocentini changed the title Add swagger and example proj (tested iOS only) Replace HTTP client with SwaggerJS Dec 21, 2016
@jamesnocentini
Copy link
Contributor Author

Sure, I think it's mergeable now. I'll create separate tickets off of your feedback. Thanks!

@jamesnocentini jamesnocentini merged commit 8bf1ad2 into master Dec 21, 2016
@jamesnocentini jamesnocentini deleted the feature/swagger branch December 21, 2016 10:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants