hoodiehq / hoodie-client

:dog: Client API for the Hoodie server
Apache License 2.0
34 stars 25 forks source link

call hoodie.store.connect() on page load if user is signed in #54

Closed gr2m closed 8 years ago

gr2m commented 8 years ago

follow up for https://github.com/hoodiehq/hoodie-client/issues/48

If the user is signed in we want to start syncing changes. The code for that looks something like this

if (hoodie.acount.isSignedIn()) {
  hoodie.store.connect()
}

And it needs to run as part of the hoodie initialisation


ransomw commented 8 years ago

so i spent a little time on this: setting up my editor for standard style, reading simple-mock, and mostly reading the hoodie source for the first time in over a year... i'd been editing test/specs/init.js for some time before i realized that this involves writing tests and implementing the connect() functionality, and i dunno if i should claim ownership of this issue as such. i will continue to watch it and try further edits.

gr2m commented 8 years ago

@ransomw if you only want to write the tests, that’s totally okay, we can split this issue up in two PRs

ransomw commented 8 years ago

so i went ahead and did both the test and implementation w/ separate PRs for each and finer-grained commits in iss54-* branches of the fork.

i feel like this was a great starter issue to get familiar w/ the collaborative process. i didn't want to muck w/ the existing tests too much, so i pretty much just matched existing patterns. one idea that occurred to me that i didn't implement, for instance, is extracting the account stub setup from the individual tests, b/c i'm not certain if this would be considered an improvement. anyway, that can be discussed in PR comments.

one thing to mention here is that i noticed the following warning on the can chain plugins calls test running on node v5.5.0:

  (node) warning: possible EventEmitter memory leak detected. 11 destroyed listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at EventEmitter.addListener (events.js:252:17)
    at setUpEventEmitter (.../hoodie-client/node_modules/pouchdb/lib/index.js:2360:9)
    at Function.PouchDB.defaults (.../hoodie-client/node_modules/pouchdb/lib/index.js:2463:3)
    at new Store (.../hoodie-client/node_modules/hoodie-client-store/index.js:45:31)
    at Store (.../hoodie-client/node_modules/hoodie-client-store/index.js:19:40)
    at new CustomStore (.../hoodie-client/node_modules/hoodie-client-store/index.js:107:12)
    at new Hoodie (.../hoodie-client/index.js:25:15)
    at Test.<anonymous> (.../hoodie-client/tests/specs/plugin.js:29:16)
    at Test.bound [as _cb] (.../hoodie-client/node_modules/tape/lib/test.js:61:32)
    at Test.run (.../hoodie-client/node_modules/tape/lib/test.js:77:10)
gr2m commented 8 years ago

great work @ransomw :clap:

we can ignore the EventEmitter warning for now. It's a built-in feature of https://nodejs.org/api/events.html to help you avoid memory leaks. We might need to bump the limit before the warning occurs, but want to implement all outstanding features and fix all known bugs first