zoepage / pouchdb-hoodie-unsynced-local-docs

check if local DB has unsynced changes
4 stars 5 forks source link

can we find a clearer name than `api.getLocalChanges()`? #4

Closed gr2m closed 9 years ago

gr2m commented 9 years ago

our API returns an array of documents that have changed locally and not yet replicated to remote. But somehow our current API reads like it would return the delta changes that have not been synced, instead of the entire documents:

api.getLocalChanges()
.then(function(changes) {
  // what is changes here?
})

I wonder if we could make it more clear that the API returns an array of changes instead of just docs, like

api.getLocallyChangedDocs()
.then(function(changedDocs) {
  // array of docs with local changes
})

Another idea would be to make an API that behaves similar to db.changes(), like

db.localChanges({
  remote: 'remoteDb',
  include_docs: true,
  // etc
})

but that feels like to low level.

Naming things is hard™ – any thoughts?

zoepage commented 9 years ago

I am with @gr2m, but getLocallyChangedDocs feels too long.

varjmes commented 9 years ago

In the Python world (something I write a lot of) we go with the "long names are cool because they explain what it is they do". So I think .getLocallyChangedDocs() is good.

+1 for naming things is super hard

janl commented 9 years ago

Hah, I love these! rubs hands in excitement

First observation: do we use get* anywhere else? Or is the PouchDB ecosystem using get* anywhere else? If not, I vote for a variant without get*.

Let’s see cracks knuckles:

I do like the mirroring go the db.changes() method with the {include_docs: true} option, but I’m not sure if this is too Couchy and not obvious enough. Maybe we need two methods:

or

I’m also +1 on @charlotteis’s point about long & descriptive names, we have a history of opting for those.

Maybe unsynced expresses what we mean better than locallyChanged, but I don’t have a clear favourite either way.

gr2m commented 9 years ago

yeah, regarding db.localChanges(), this could actually be another plugin, that will be used within ours here. But we want a higher level API for Hoodie.

Also great point on the get prefix. In fact, in PouchDB land, they don't do that. It's just db.info(), not db.getInfo(), all methods are async by convention. But I think this is a somewhat higher level method, similar to what we did in pouchdb-hoodie-api, they also don't follow PouchDB conventions really. Instead the API should work well on hoodie.store. So I lean towards

.getLocallyChangedDocs(), or .getLocallyChanged().

unsynced doesn't feel as clear to me. Also not that the method accepts an id, object, or array of both as an argument, for filtering.

varjmes commented 9 years ago

Please listen to this while thinking of names: https://www.youtube.com/watch?v=Eo-KmOd3i7s

I quite like db.unsyncedDocs()

janl commented 9 years ago

@gr2m what’s your take on locallyChanged vs unsynced? My idea here is that a “local change” needs a reference what “local” is and what can “change”, whereas “sync”, while seemingly similar, is a more familiar concept, especially given that we already have “sync”-APIs.

janl commented 9 years ago

But I could be way off on what feels more familiar to a newcomer.

zoepage commented 9 years ago

for me, unsynced implies a sync, but locallyChanged would be independent from this thought. You could also just delete the docs, you've not synced yet...

With locallyChanged it's not quite sure, what you get back... The docs? Just if there were changes? How many?

I'd pref the longer name then, when it is more clear.

gr2m commented 9 years ago

you are right, unsynced is more descriptive. Also I trust native speaker @Charlotteis much more than my gut feeling :)

Maybe we should just call it db.unsynced, and allow to pass in include_docs and keys options, just like changes. And then for our own use in hoodie.store, we make a tiny wrapper and call it hoodie.store.findAllUnsynced() or similar. That would fit nicely with the rest of the api, better than getLocalChanges() I think

varjmes commented 9 years ago

+1 to your thoughts there, @gr2m

janl commented 9 years ago

@gr2m does hoodie.store.findAll() take an options argument? I’m asking for hoodie.store.findAll({unsynced: true}); — no worries if not, hoodie.store.findAllUnsynced() is fine :)

gr2m commented 9 years ago

The discussion on hoodie.store.findAllUnsynced() is not priority right now, there are some other things we have to discuss before adding this to feature to hoodie.store anyway.

But discussing about a PouchDB plugin. Shall we call it pouchdb-hoodie-unsynced, and have it add a method db.unsynced(), that accepts the options remote: "remotedb", include_docs: true and keys? That's what I tend to do right now. I feel like that'd be useful to the PouchDB eco system anyway

zoepage commented 9 years ago

-1

janl commented 9 years ago

@gr2m that would get my vote because it feels PouchDB-y, which is what we want from a PouchDB plugin.

I’d be confident that the PouchDB community will send us any feedback on what we could do better API wise, but my bet is, that by mirroring it so closely, it’ll just be a natural fit.

janl commented 9 years ago

@zoepage -1s must be qualified :D — Why do you think this is not a good idea?

zoepage commented 9 years ago

sorry, I was still editing.

liked mentioning above, the naming unsynced implies just a sync is possible. You get back the docs which are just saved locally and can do other things like edit, delete etc.

this is why i don't like the wording.

on the method accepting options, yes. this would feel more pouchdb-y, but would be low level. I thought, we want to abstract on a high level.

lewiscowper commented 9 years ago

(a) I would always defer to unsynced instead of unsynched in matters of spelling

(b) I feel that unsynced is a reasonable abstraction rather than locallyChangedDocs and derivatives of it. We (hoodie) have a reliable contract within the application that if you ask to store something for your application, the first place it goes is somewhere within your browser. Whether that is then later synced is inconsequential to the user at the point of storage, only the point of retrieval with a new device or session.

(c) I'm not sure I have the same feeling as @zoepage over whether unsynced implies that a sync is possible, but it becomes a semantic difference.

I think that exposing the idea of a "doc" to the dev doesn't feel very Hoodie-ish to me, especially in an API. (That could also mean I disagree with other API naming choices, this is just the first thread that I've been on time to comment for). The end result from the API, (as far as I can tell by reading this thread), is a list of the local changes.

I think a reasonable thing to do with those changes is to sync them if you have a remote node already set up, so I think unsynced fits really well to that use case, and explains what you're getting back easily.

janl commented 9 years ago

It looks like we need to clarify at which level this is supposed to live:

  1. This is a PouchDB plugin, that Hoodie uses, but that anyone else can use as well (in which case it should be called pouchdb-unsynced (or whatever) and mirror PouchDB APIs as close as possible ({include_docs: true} and whatnot).
  2. This is a PouchDB plugin that Hoodie uses that also provides a Hoodie-user-facing API method called locallyChangedDocs or unsynced or whatever. At which point, the API should be as friendly and Hoodie-ish as possible.
  3. This is a PouchDB plugin that Hoodie uses that provides an API to another Hoodie module that then in turn creates a user facing API method. In which case, the API should mirror other Hoodie internals and doesn’t necessarily have to be as user friendly.

Or anything in between, which is it? :)

Also, what is the exactly API intention: return a list of doc ids for docs that have not yet been synced to the remote and optionally with the full doc bodies? Or only the latter?

nolanlawson commented 9 years ago

I like getUnsyncedDocs(). But as @janl said, it does depend on what level this thing is supposed to live at.

For feeling "un-Pouchy," go with whatever your heart says. :) I've written some PouchDB plugins that have a decidedly un-Pouchy feel to them.

janl commented 9 years ago

@zoepage I’m not sure I follow, but maybe this will clear it up: in CouchDB (and hence in my head) even doc deletes and edits are just new states of a doc that need to get synced. I can’t say if this is obvious to non-CouchDB folks, hence my question.

Aside from that though, my thinking is this: The Hoodie Store is a mirror of a database that lives on the server. Docs can be added, edited and deleted on either side. A sync operation makes that all things from one side go to the other, or vice versa, or both.

On either side, it is useful to see, whether any of the additions, edits or deletions have made it to the other side yet or not:

In that view of Hoodie Store, “sync” is a natural operation “unsynced” feels natural to me.

But I’m decidedly not coming at this from a beginner’s perspective, so I am very likely missing something and that’s why I’d like to get @zoepage on board with this, because she has a lot of experience with first-time Hoodie users and their mind-set.

gr2m commented 9 years ago

From hoodie.store prespective, we need an API for this question:

To explain where we are coming from:

  1. Right now, we have a method hoodie.store.hasLocalChanges(). I think it's great, and it's synchronous, but we decided to replace this with an async method, to avoid custom state keeping of which docs have been synced and which are pending, and storing this state manually in localStorage. Instead, we only rely on what PouchDB gives us already.
  2. We have two other PouchDB plugins, pouchdb-hoodie-api and pouchdb-hoodie-sync, which both provide several methods, that are high level, and are 1:1 of what we will expose at hoodie.store.
  3. pouchdb-hoodie-local-changes wasn't planned initially, we just came up with making this extra plugin as a proof of concept. We followed lead of exposing a high level API like in the other two plugins.

I'm not sure that we should still try to expose a high level method that will be 1:1 like that in hoodie.store. I changed my mind here. For the other two plugins it makes sense, but here, it's one single method. I'd go with pouchdb-unsynced, make it very close to other PouchDB APIs, and do the higher level wrapping right in hoodie-pouchdb-store, which combines all the PouchDB plugin into one integrated store API.

So I'd suggest to change course to where @zoepage and I originally came from, which creates extra work and was unexpected, but I think it's worth it in this case.

janl commented 9 years ago

@nolanlawson thanks for your input, this is useful :) — I definitely think there is a place for non-Pouchy plugins, and yours is a great example. I just think for relatively straightforward things like localChanges, there is no harm in making it feel “at home” as much as possible, if we (Hoodie) intend other people to benefit from this.

varjmes commented 9 years ago

Hmmm. I do understand where @zoepage is coming from now on how limited unsynced might appear to be. I do think it's a good term, but I'd really like to hear what @zoepage thinks would be more suitable.

I have very little experience with Hoodie internals, unsynced makes sense to me but that might be because I've been part of this discussion, where others will not be when they come to use it :)

janl commented 9 years ago

@Charlotteis Maybe try this: imagine you are building an app, you know very little about Hoodie, but you know that data goes in and magically arrives at the server somehow, and back, if needed. Is “sync” a concept that is front and center for you now, or is it some magic thing that you don’t care about because you just want to get this thing to work, where you need to know which of your things you put into hoodie are not yet on the server.

varjmes commented 9 years ago

@jan As someone who does know very little about hoodie, it's not hard to imagine :D. I guess if I was not having this conversation, I would like things to magically work and then later I might look into the internals.

zoepage commented 9 years ago

After sleeping about it, I'm still not happy with unsyncedDocsfor one reason: We just check the locally changed docs, not locally and remote. This implies the word sync for me. This is why local/ locally is a keyword, I'd love to keep.

On the point, if we expose the method on a high-level... From a personal point of view, I love to have this method. It gives so much more possibilities to the user / dev. Especially with keeping the "offline-first" concept in mind and giving the user / dev all the power to develop an application they want.

But from a hoodie developer point of view, I don't mind to keep the method on the low-level. We want to make the user / dev experience, as simple as possible.

So for me, this is a lot about: how much power we want to give to the user / dev vs how simple we want hoodie to be.

varjmes commented 9 years ago

@zoepage's point about local/remote have sold it for me. +1 for local/locally.

janl commented 9 years ago

After sleeping about it, I'm still not happy with unsyncedDocsfor one reason: We just check the locally changed docs, not locally and remote. This implies the word sync for me. This is why local/ locally is a keyword, I'd love to keep.

The implementation of this method does in fact track the state of the client and the server to determine which docs have not yet been synced. So it actually does exactly this: check client and server.

On the point, if we expose the method on a high-level...

I don’t think there’s a question whether we expose function like this to the end-user. Hoodie already has it, it is useful, we should keep it. All we are discussing is whether this particular method is an internal API (doesn’t have to be 100% user friendly) or an external, user-facing API (must be 100% user friendly) and what it should be called and look like depending on whether it is internal or external.

daleharvey commented 9 years ago

From a PouchDB perspective my main worry about this api is the arguments need to be identical to the call to replicate for it to work, my first impression would be that

var replication = db.replicate(remote, {filter: someFilter});
console.log(replication.id);

On that being mentioned, @gr2m mentioned he doesnt want to actually start the replication, so in that case

var replication = db.replicate(remote, {filter: someFilter, start: false});

Seems like the most sane api as to what we be introducing to pouchdb core, obviously anything can go on top of that, personally I would avoid references to local / remote since although they are commonly people are replicating from a local to a remote db, there is no requirement for them to be doing so

zoepage commented 9 years ago

The implementation of this method does in fact track the state of the client and the server to determine which docs have not yet been synced. So it actually does exactly this: check client and server.

This is not exactly what I ment... It doesn't give back the changedDocs from remote, does it? :) Maybe my insight in here, isn't as deep as it should be.

All we are discussing is whether this particular method is an internal API (doesn’t have to be 100% user friendly) or an external, user-facing API (must be 100% user friendly) and what it should be called and look like depending on whether it is internal or external.

This pretty much sums up what I wanted to express. Thank you :)

gr2m commented 9 years ago

@daleharvey db.replicate(remote, {filter: someFilter, start: false}).id would be very helpful indeed, I see https://github.com/hoodiehq/pouchdb-hoodie-local-changes/blob/master/lib/get-local-changes.js#L3 only as a temporary hack.

main worry about this api is the arguments need to be identical to the call to replicate for it to work

This is a known flaw at this point, the API is kinda lying, as filtered replications are ignored, so our API might return docs that actually have been replicated. I'd like to discuss this later to not side track the discussion at hand if you don't mind?

janl commented 9 years ago

This is not exactly what I ment... It doesn't give back the changedDocs from remote, does it? :) Maybe my insight in here, isn't as deep as it should be.

NOW I think I get your point. Finally, thanks :)

Ok, I will have to think about this for a while :D

gr2m commented 9 years ago

One more thing that came to my mind is that "local changes" might be confusing in a PouchDB API, because "changes" has a semantic meaning already, as it's a core API db.changes(), while "unsynced" does not have that issue.

If db.unsynced() is not clear enough, we could call it db.unsyncedLocal() or db.unsyncedLocalDocs(). I'd still tend to db.unsynced, I think it's a great name & API, and we'd make clear what it is & does in the README.

zoepage commented 9 years ago

I'd be down with 'db.unsyncedLocalDocs' ;)

gr2m commented 9 years ago

Yeah I could live with that. And it's very clear, too :)

The full API would require options.remote, and when I compare

db.unsyncedLocalDocs({
  remote: 'http://localhost:5984/testdb'
})

with

db.unsynced({
  remote: 'http://localhost:5984/testdb'
})

I agree that just db.unsynced could maybe be unclear that the unsynced docs in the local db are returned, not the one from the remote db.

zoepage commented 9 years ago

Right, didn't thought about the options that way before. Good catch. :)

Any objections @Janl and others? :)

varjmes commented 9 years ago

Sounds good to me :)

gr2m commented 9 years ago

discussed with @zoepage, that'd be the API with all options:

db.unsyncedLocalDocs({
  remote: 'http://localhost:5984/testdb',
  // optional
  keys: [id1, id2]
}).then(function (docs) {
  // docs is array of docs
})
zoepage commented 9 years ago

I understood it like this:

db.unsyncedLocalDocs({
  remote: 'http://localhost:5984/testdb',
  include_docs: true,
  // optional
  keys: [id1, id2]
}).then(function (docs) {
  // docs is array of docs
})
´´´´
gr2m commented 9 years ago

if we add a include_docs: true, option, it would mean that it can be set to false, then the method name would be confusing because it somewhat implies that it returns entire docs, always.

So internally, I'd always set include_docs: true on the db.changes() call, but making this an option for db.unsyncedLocalDocs doesn't make sense I think. If we want that, we should rename the method to db.unsyncedLocal :)

zoepage commented 9 years ago

This is what I ment. Sorry for the confusion :)

gr2m commented 9 years ago

:ok_hand:

janl commented 9 years ago

Love this, good job everyone! :)

zoepage commented 9 years ago

one thought came to my mind: Module name unsyncedLocalDocs :white_check_mark: Method name unsyncedLocalDocs or getUnsyncedLocalDocs

cc @gr2m && @janl

janl commented 9 years ago

Do you mean pouchdb-hoodie-unsyncedLocalDocs for the module name?

Method name: either works. I explained my thoughts about get* in https://github.com/hoodiehq/pouchdb-hoodie-local-changes/issues/4#issuecomment-118150849

zoepage commented 9 years ago

re:get okay, cool. Just wanted to be sure :)

re:module name following the usual conventions, it would be pouchdb-hoodie-unsynced-local-docs

janl commented 9 years ago

@zoepage okay, then I don’t understand your:

Module name unsyncedLocalDocs :white_check_mark:

:)

zoepage commented 9 years ago

var exports = module.exports = { unsyncedLocalDocs: unsyncedLocalDocs }

janl commented 9 years ago

That’s a purely internal name, so it doesn’t matter, but yeah, sounds reasonable.

janl commented 9 years ago

thanks for the explanation