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

Compatibility with PouchDB 7 #254

Merged

Conversation

leonid-shevtsov
Copy link
Contributor

@leonid-shevtsov leonid-shevtsov commented Apr 21, 2019

I've seen #238, but it looked abandoned, and there was refactoring seemingly unrelated to PouchDB 7. I figured it's easier to start over. Sorry about that.

  • upgrade dependencies to PouchDB 7
  • drop dependency on deprecated pouchdb-ajax and pouchdb-promise
  • for tests, replace deprecated (or just neglected?) pouchdb-memory build with pouchdb-browser and pouchdb-node.
  • for the best integration with the ecosystem, we must use db.fetch, and not any other instance of fetch (such as an additional polyfill, or even imported from pouchdb-fetch). This will let this plugin cooperate with other plugins that might customize fetch behavior. So:
    • we shouldn't directly depend on the pouchdb-fetch package, since it's baked into pouchdb-browser and other builds.
  • since fetch works with promises naturally, it's more appropriate to define the API in terms of promises as well. So to support the callback style, I've defined a toCallback helper which does roughly the opposite of toPromise and enables both the promise and the callback API styles.
  • I've implemented fetchJSON - a wrapper around fetch that receives and returns JSONs. Then I've looked and, of course, pouchdb-adapter-http has a very similar function, but it's private. Perhaps some day we can extract and reuse it.
  • tests have barely changed at all, as you can see.

Needless to say, this changeset is not compatible with PouchDB 6 anymore.

@leonid-shevtsov
Copy link
Contributor Author

leonid-shevtsov commented Apr 21, 2019

There are some issues in the tests:

  • NodeJS tests are failing due to the wrong fetch-cookie version; either pouchdb-node will be updated, or we can make a custom build and use the correct fetch-cookie. Though until pouchdb-node is updated, pouchdb-authentication will still error against the official build, so maybe no sense in doing anything custom.
  • PhantomJS probably needs a fetch polyfill
  • and Sauce tests show some issues with running against a CouchDB instance. I've tested locally on Chrome + pouchdb-server and everything was green - I'll see if I can repro against CouchDB.

@jlami
Copy link

jlami commented May 6, 2019

Can we use the api.fetch that pouchdb-adapter-http exposes?
https://github.com/pouchdb/pouchdb/blob/858660e9b37959d96eebf667ea9b8b1273f0d62c/packages/node_modules/pouchdb-adapter-http/src/index.js#L437
That one also supports 'overloading' so a user can give their own fetch function to modifiy the request.

We recently found out that we needed to overrule a signup request when logged in as a non-admin user to be able to prevent credentials from being included. I don't really see a way of doing this with your setup.

If we switched to api.fetch it might also prevent the need for getBasicAuthHeaders, since those seem to be handled there:
https://github.com/pouchdb/pouchdb/blob/858660e9b37959d96eebf667ea9b8b1273f0d62c/packages/node_modules/pouchdb-adapter-http/src/index.js#L169-L176

@leonid-shevtsov
Copy link
Contributor Author

leonid-shevtsov commented May 18, 2019

@jlami good point.

I was already using the fetch from pouchdb-adapter-http, which is available through PouchDB.fetch and in db.fetch, so it did support overloading

However, I did some digging and turns out, if you use db.fetch, you can pass it paths instead of URLs, and it will build the URL for you. And as you said, it also sets auth headers and enables cookies. This removes a lot of code from pouchdb-authentication.

Unfortunately this PR won't work until there's a new version of PouchDB released; this fix in particular is absolutely necessary.

@leonid-shevtsov
Copy link
Contributor Author

leonid-shevtsov commented May 19, 2019

I made a PR to pouchdb to expose the fetchJSON API so I can remove the implementation I made in this one.

pouchdb/pouchdb#7768

@mqtik
Copy link

mqtik commented Sep 4, 2020

Any updates on this?

@ptitjes
Copy link
Collaborator

ptitjes commented Dec 29, 2022

Thank you @leonid-shevtsov. Sorry for the very late merge...

@leonid-shevtsov
Copy link
Contributor Author

No worries! Thank you so much for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants