romgrk / node-todoist

nodejs implementation of todoist sync API
21 stars 12 forks source link

batch commits #12

Open PieterDePauw opened 4 years ago

PieterDePauw commented 4 years ago

Hi all,

First of all ... what an excellent module, everybody! Thanks for your hard work!

However, in my project I will often need to update a lot of tasks at once. Because of that, batching multiple commands would probably provide a better experience. I saw your comment on the README.md about this, and I was wondering whether you are considering adding this functionality.

Kind regards, P

lordnox commented 4 years ago

Currently neither I nor @romgrk are writing features. For both our use-cases the current state is enough.

Do you have a proposal how an API could look like?

romgrk commented 4 years ago

If you have more info on your use-case we could get something out.

Simple batched commands with no guarantees on the inner state of the API instance would be easy to implement, we can simply accumulate them instead of executing them in createCommand, and make api.commit() do the execution. The .get() functions could however not be considered the source of truth while there are batched commands. Complex commands using temporary IDs would however require much more work (eg creating a project and adding a note to it in the same batch).

// Proposition 1
const api = Todoist(key, { batched: true })
api.items.add({ content: 'Task 1' })
api.items.add({ content: 'Task 2' })
api.commit()
// Proposition 2
const api = Todoist(key)
api.commit(() => {
  api.items.add({ content: 'Task 1' })
  api.items.add({ content: 'Task 2' })
})

Ideally the instance could do the bookkeeping and be a source-of-truth even while there are batched commands. The sync API is conceived for mobile use-cases, because it allows incremental updates and offline-use, but this would require quite a bit of work and isn't planned for the moment.

rcodesmith commented 3 years ago

I'm needing something like this as well. To simplify things, would requiring a synchronization afterwards to update the api's state help? That would be fine for my use case.

e.g.

const api = Todoist(key)
await api.commit(() => {
  api.items.add({ content: 'Task 1' })
  api.items.add({ content: 'Task 2' })
});
// api state not consistent with Todoist here
await api.sync();
// Now api state is consistent.
romgrk commented 3 years ago

So essentially what you need is just to be able to batch operations, without the need to maintain an un-synced from Todoist state. Is that right?

In that case, it is indeed not so much work to implement. The api.commit() call could take care of calling api.sync() right away.

PieterDePauw commented 1 year ago

I would mainly use this kind of feature to prevent an unnecessarily high number of network requests – that could potentially result in hitting the rate limit. Instead of executing all changes immediately by calling api.sync(), my strategy would be to synchronise updates frequently but based on a strictly timed schedule (f.i. one request every few seconds) and, in the meanwhile, I'd capture the user (inter)actions on my side and batch all commands together to execute everything as a single request. I am not entirely sure whether that is indeed the best approach of this problem however.

So, what do we think that the best solution would be to implement this feature?

romgrk commented 1 year ago

I think proposition 1 above would solve your issue best. I don't have enough time or interest to implement the feature myself, I've mostly stopped using todoist. If you're interested in adding it yourself, this module is really just a thin layer over the REST API, which translates calls from api.items.add into their got(...) equivalent. The core logic is less than 150 lines, most of the 400 lines of code we have is just mapping API calls.

PieterDePauw commented 1 year ago

Yes, I definitely will attempt to implement this functionality myself. As you pointed out, the core request logic is indeed not very difficult to understand and actually quite recognizable. I was initially a bit confused, but it turns out that the reason was mainly that I am absolutely not familiar with the structures of got.

At this time, I think that I just will try to proceed by accumulating batched commands together in a commands parameter. I can also see how an un-synced state could be used, but I personally think that possible ways of resolving the differences afterwards between the state and the server might add too much complexity compared to the advantages. Keeping the state as close as possible to the actual server state is much more appealing to me.

As a sidenote: is the value of autocommit intended for batched commands or un-synced state? Because I was not yet able to figure out any other purpose.

romgrk commented 1 year ago

Sounds like a good plan. I'm really not attached to got, it could be isomorphic-fetch, doesn't really matter. If I were you, I'm not sure how much I'd invest in doing smart resolution of the server state after the fact. I'd try to solve the easiest/simplest use-case possible, doing otherwise might introduce bugs in the client, whereas I think it's nice to keep it simple.

I don't remember why I added autocommit, probably in preparation for something like this. But it's not used or documented anywhere, feel free to change it. We can do a major semver release anyway if need be.