feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks
Summary:
This diff does a couple things:
- Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable.
- NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request.
- Aborting a NylasAPI request now makes it call it's error callback / promise reject.
- You can now run code after perform local has completed using this syntax:
```
task = new AddRemoveTagsTask(focused, ['archive'], ['inbox'])
task.waitForPerformLocal().then ->
Actions.setFocus(collection: 'thread', item: nextFocus)
Actions.setCursorPosition(collection: 'thread', item: nextKeyboard)
Actions.queueTask(task)
```
- In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0).
- The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote".
- All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later.
- API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish.
- The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon.
Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero.
Known Issues:
- AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other.
- When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar.
- The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first!
Test Plan: All the tests still pass!
Reviewers: evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
|
|
|
Actions = require '../../src/flux/actions'
|
|
|
|
|
TaskQueue = require '../../src/flux/stores/task-queue'
|
|
|
|
|
Task = require '../../src/flux/tasks/task'
|
|
|
|
|
|
|
|
|
|
{APIError,
|
|
|
|
|
OfflineError,
|
|
|
|
|
TimeoutError} = require '../../src/flux/errors'
|
|
|
|
|
|
|
|
|
|
noop = ->
|
|
|
|
|
|
|
|
|
|
describe "Task", ->
|
|
|
|
|
describe "initial state", ->
|
|
|
|
|
it "should set up queue state with additional information about local/remote", ->
|
|
|
|
|
task = new Task()
|
|
|
|
|
expect(task.queueState).toEqual({ isProcessing : false, localError : null, localComplete : false, remoteError : null, remoteAttempts : 0, remoteComplete : false })
|
|
|
|
|
|
|
|
|
|
describe "runLocal", ->
|
|
|
|
|
beforeEach ->
|
|
|
|
|
class APITestTask extends Task
|
|
|
|
|
performLocal: -> Promise.resolve()
|
|
|
|
|
performRemote: -> Promise.resolve(Task.Status.Finished)
|
|
|
|
|
@task = new APITestTask()
|
|
|
|
|
|
|
|
|
|
describe "when performLocal is not complete", ->
|
|
|
|
|
it "should run performLocal", ->
|
|
|
|
|
spyOn(@task, 'performLocal').andCallThrough()
|
|
|
|
|
@task.runLocal()
|
|
|
|
|
expect(@task.performLocal).toHaveBeenCalled()
|
|
|
|
|
|
|
|
|
|
describe "when performLocal rejects", ->
|
|
|
|
|
beforeEach ->
|
|
|
|
|
spyOn(@task, 'performLocal').andCallFake =>
|
|
|
|
|
Promise.reject(new Error("Oh no!"))
|
|
|
|
|
|
|
|
|
|
it "should save the error to the queueState", ->
|
|
|
|
|
@task.runLocal().catch(noop)
|
|
|
|
|
advanceClock()
|
|
|
|
|
expect(@task.performLocal).toHaveBeenCalled()
|
|
|
|
|
expect(@task.queueState.localComplete).toBe(false)
|
|
|
|
|
expect(@task.queueState.localError.message).toBe("Oh no!")
|
|
|
|
|
|
|
|
|
|
it "should reject with the error", ->
|
|
|
|
|
rejection = null
|
|
|
|
|
runs ->
|
|
|
|
|
@task.runLocal().catch (err) ->
|
|
|
|
|
rejection = err
|
|
|
|
|
waitsFor ->
|
|
|
|
|
rejection
|
|
|
|
|
runs ->
|
|
|
|
|
expect(rejection.message).toBe("Oh no!")
|
|
|
|
|
|
|
|
|
|
describe "when performLocal resolves", ->
|
|
|
|
|
beforeEach ->
|
|
|
|
|
spyOn(@task, 'performLocal').andCallFake -> Promise.resolve('Hooray')
|
|
|
|
|
|
|
|
|
|
it "should save that performLocal is complete", ->
|
|
|
|
|
@task.runLocal()
|
|
|
|
|
advanceClock()
|
|
|
|
|
expect(@task.queueState.localComplete).toBe(true)
|
|
|
|
|
|
|
|
|
|
it "should save that there was no performLocal error", ->
|
|
|
|
|
@task.runLocal()
|
|
|
|
|
advanceClock()
|
|
|
|
|
expect(@task.queueState.localError).toBe(null)
|
|
|
|
|
|
|
|
|
|
describe "runRemote", ->
|
|
|
|
|
beforeEach ->
|
|
|
|
|
@task.queueState.localComplete = true
|
|
|
|
|
|
|
|
|
|
it "should run performRemote", ->
|
|
|
|
|
spyOn(@task, 'performRemote').andCallThrough()
|
|
|
|
|
@task.runRemote()
|
|
|
|
|
advanceClock()
|
|
|
|
|
expect(@task.performRemote).toHaveBeenCalled()
|
|
|
|
|
|
|
|
|
|
describe "when performRemote resolves", ->
|
|
|
|
|
beforeEach ->
|
|
|
|
|
spyOn(@task, 'performRemote').andCallFake ->
|
|
|
|
|
Promise.resolve(Task.Status.Finished)
|
|
|
|
|
|
|
|
|
|
it "should save that performRemote is complete with no errors", ->
|
|
|
|
|
@task.runRemote()
|
|
|
|
|
advanceClock()
|
|
|
|
|
expect(@task.performRemote).toHaveBeenCalled()
|
|
|
|
|
expect(@task.queueState.remoteError).toBe(null)
|
|
|
|
|
expect(@task.queueState.remoteComplete).toBe(true)
|
|
|
|
|
|
|
|
|
|
it "should only allow the performRemote method to return a Task.Status", ->
|
|
|
|
|
result = null
|
|
|
|
|
err = null
|
|
|
|
|
|
|
|
|
|
class OKTask extends Task
|
|
|
|
|
performRemote: -> Promise.resolve(Task.Status.Retry)
|
|
|
|
|
|
|
|
|
|
@ok = new OKTask()
|
|
|
|
|
@ok.queueState.localComplete = true
|
|
|
|
|
@ok.runRemote().then (r) -> result = r
|
|
|
|
|
advanceClock()
|
|
|
|
|
expect(result).toBe(Task.Status.Retry)
|
|
|
|
|
|
|
|
|
|
class BadTask extends Task
|
|
|
|
|
performRemote: -> Promise.resolve('lalal')
|
|
|
|
|
@bad = new BadTask()
|
|
|
|
|
@bad.queueState.localComplete = true
|
|
|
|
|
@bad.runRemote().catch (e) -> err = e
|
|
|
|
|
advanceClock()
|
|
|
|
|
expect(err.message).toBe('performRemote returned lalal, which is not a Task.Status')
|
|
|
|
|
|
|
|
|
|
describe "when performRemote rejects", ->
|
|
|
|
|
beforeEach ->
|
|
|
|
|
@error = new APIError("Oh no!")
|
|
|
|
|
spyOn(@task, 'performRemote').andCallFake => Promise.reject(@error)
|
|
|
|
|
|
|
|
|
|
it "should save the error to the queueState", ->
|
|
|
|
|
@task.runRemote().catch(noop)
|
|
|
|
|
advanceClock()
|
|
|
|
|
expect(@task.queueState.remoteError).toBe(@error)
|
|
|
|
|
|
|
|
|
|
it "should increment the number of attempts", ->
|
|
|
|
|
runs ->
|
|
|
|
|
@task.runRemote().catch(noop)
|
|
|
|
|
waitsFor ->
|
|
|
|
|
@task.queueState.remoteAttempts == 1
|
|
|
|
|
runs ->
|
|
|
|
|
@task.runRemote().catch(noop)
|
|
|
|
|
waitsFor ->
|
|
|
|
|
@task.queueState.remoteAttempts == 2
|