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

merge pouchdb-hoodie-api and pouchdb-hoodie-sync into hoodie-store-client #121

Closed
14 tasks done
gr2m opened this issue Jun 17, 2017 · 1 comment · Fixed by hoodiehq/hoodie-store-client#153
Closed
14 tasks done

Comments

@gr2m
Copy link
Member

gr2m commented Jun 17, 2017

🤔 What you will need to know

First, this is a rather advanced issue. If you are new to Hoodie, it might not be a good first issue to work on. But if you feel confident, don’t let that stop you :)

When we decided to rebuild Hoodie from scratch, the most important change was to move away from our self-cooked localStorage based sync engine and use PouchDB. The Store module was our first milestone, and creating a hoodie-like API based on PouchDB was our first step. The result was pouchdb-hoodie-api, a PouchDB plugin.

The challenge with it is that the API returned by db.hoodieApi() is bound to the current PouchDB db instance. Once the db is destroyed, the API can no longer be used. And we destroy the db e.g. when a user signs out or signs in to another account. We delete the local store database and recreate it.

In order to not need to re-create the hoodie.store.* api each time that happens, we created @hoodie/store-client. It takes the PouchDB constructor and it keeps a local state for the current db instance. But once it gets destroyed, we simply replace it with another one, without bothering the user about it. They can simply continue to use APIs like hoodie.store.add(doc), hoodie.store.on(eventName, handler), etc.

@hoodie/store-client is using pouchdb-hoodie-api and pouchdb-hoodie-sync internally (see @hoodie/store-client/index.js.

❓ The Motivation

The separation caused quite some headache in the past, and I think it’s time we consolidate. The current issue is hoodiehq/hoodie-store-client#149.

There is also some code duplication, e.g. both pouchdb-hoodie-api and pouchdb-hoodie-sync define .on methods and we pass an EventEmitter and it’s all unnecessary complicated, error prone and prone to memory leaks.

🎯 The goal

The goal is to deprecate pouchdb-hoodie-api and pouchdb-hoodie-sync and merge all its code and tests into hoodie-store-client.

📋 Step by Step

  • 🙋 Claim this issue: Comment below (or assign yourself and continue at step 4 :)
    Please 🙏 only claim if you want to start working on it within a day.
    Once claimed we add you as contributor to this repository.
  • 👌 Accept our invitation to this repository. Once accepted, assign yourself to this repository
  • 👓 Please review our Code of Conduct
    In a nutshell: be patient and actively kind with us 😊
  • 🔄 replace the up for grabs label with in progress.
  • 🔢 Setup the 3 repositories: pouchdb-hoodie-api, pouchdb-hoodie-sync, hoodie-store-client Make sure that the existing tests pass
  • merge pouchdb-hoodie-api. Start a PR as soon as you have start working on it, it’s easier to help you when discussing a pull request directly if you get stuck :) Mention closes hoodiehq/camp#121 in the description.
    • move all lib/* files over to hoodie-store-client/lib/*
    • move logic from pouchdb-hoodie-api/index.js where we start listening to PouchDB’s changes feed to emit our events.
    • remove pouchdb-hoodie-api from hoodie-store-client/package.json and hoodie-store-client/index.js and adapt the code where we put together the api.
    • move all tests. Make sure the tests all pass after moving to hoodie-store-client.
  • merge pouchdb-hoodie-sync the same way
  • Now add the test from WIP - scoped store after .reset() hoodie-store-client#150, it should fail
  • Figure out how to make the test pass :) The API returned by .withIdPrefix() must not be bound to the current db instance. When I create a store API which has an ID prefix, than that api should continue to work even after a user signs out or signs in, meaning the db instance in the API that .withIdPrefix() returns must be replaced in reset.js somehow.
  • 🏁 Done 👍 Replace the in progress label with ready. Ask in comments for a review :)

🤔❓ Questions

Ping us in the Hoodie Chat or on Twitter

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

Successfully merging a pull request may close this issue.

1 participant