mattkrick / cashay

:moneybag: Relay for the rest of us :moneybag:
MIT License
453 stars 28 forks source link

Delete mutations and their related queries #146

Closed dustinfarris closed 7 years ago

dustinfarris commented 7 years ago

I have a scenario where user enters on a item index route

/items

The user clicks on the first item taking it to the detail route

/items/1

There is a delete button which the user clicks.

The delete action does a mutation

cashay.mutate('deleteItem', { variables: { itemId: 1 } });

and then redirects to the index route

/items

The item is still there.

My first thought, "this needs a mutation handler"

So I add one to the index route query:

const { data: { items } } = cashay.query(itemsQuery, {
  op: 'IndexRoute',
  mutationHandlers: {
    deleteProject(optimisticVariables, queryResponse, currentResponse) {
      debugger;  // What is going to happen here?  I'm not sure yet
    }
  }
}

The debugger never fires. Right before the handler is executed, I get this error:

Uncaught Error: Cache went stale & wasn't recreated. Did you forget to put a redux subscriber on IndexRoute?

I'm not sure how to interpret this.

mattkrick commented 7 years ago

keep going! you'll need to build out the mutation handler.

maybe something like

if (optimisticVariables) {
  idx = currentResponse.findIndex(x => x.id === 1)
  currentResponse.splice(idx,1)
  return currentResponse
}

don't worry about immutability. Cashay manages its own dependencies so it'll normalize this, diff it, figure out what things changed, and then invalidate those components.

dustinfarris commented 7 years ago

Thanks, but my debugger does not fire and it appears that the handler is never called.

The error occurs here: https://github.com/mattkrick/cashay/blob/master/src/Cashay.js#L594

Just a few lines before the first call to a mutation handler. Updating the mutation handler as you've suggested does not seem to be executed.

mattkrick commented 7 years ago

that's probably because you don't have a deleteItem mutation handler. it says deleteProject?

dustinfarris commented 7 years ago

No sorry that's just a crap attempt to edit for the GitHub issue. Everything is "project" in my library and the names do line up.

Sent from my iPhone

On Dec 7, 2016, at 7:37 PM, Matt Krick notifications@github.com wrote:

that's probably because you don't have a deleteItem mutation handler. it says deleteProject?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

dustinfarris commented 7 years ago

Here's screenshots

The action handler when the user clicks the delete button:

The index route stateToComputed

dustinfarris commented 7 years ago

A little more info, here's a screenshot at the time just before the error is thrown

screenshot

Indeed, there is no cached query for IndexRoute, but there should be! This is after I've visited that page (and hence run that cashay query). So it should be there. Still investigating why it has disappeared.

dustinfarris commented 7 years ago

Ok, I see what's happening now.

When I visit the index page, I fire off the "IndexRoute" query via cashay. When cashay gets the data back, it caches it in cachedQueries.IndexRoute.responses.

When I click on one of the projects, I am taken to the project detail page, which fires a new "ProjectDetail" query via cashay.

When the response for this second query comes back, cashay executes flushDependencies which removes the "IndexRoute" cached query.

Then I hit the delete button which triggers a mutation. The mutation sees that it has a mutation handler in the IndexRoute query and starts that process. Before executing the mutation handler, cashay checks for cached queries for that op, "IndexRoute", and throws an error when it doesn't find any.

@mattkrick I believe I have traced what is happening, but I'm not sure why it is happening. Can you offer some guidance?

mattkrick commented 7 years ago

When the response for this second query comes back, cashay executes flushDependencies which removes the "IndexRoute" cached query.

This is good! This is basically cashay's form of memory management. it aggressively invalidates responses that are no longer in the view layer. Since we aren't sure if the user will travel back to /item we don't create it right now, we lazily wait until they hit /item again.

Then I hit the delete button which triggers a mutation. The mutation sees that it has a mutation handler in the IndexRoute query and starts that process.

this is a really interesting use case (deleting yourself from your own leaf). that makes this super interesting!

so the easy answer:

this.transitionToRoute('index');
cashay.mutate(...)

I'm all about frameworks that don't let you write bad code. If cashay allowed you to mutate first, then it'd delete the data that is currently on the screen, which would leave you with an empty screen just before it redirected. Depending on how ember renders to the DOM, on slower machines you might see a flash of empty data before a redirect. By forcing you to redirect first, your user gets a cleaner experience with fewer rerenders and you get a more performant app. Granted, the error message sucks because there are many reasons that you might be seeing this.

If you have a good reason to mutate & then transition, I'd love to hear it! It wouldn't be difficult to write a fix for that use case (recreate the query if it is undefined) but that would cause a few unnecessary CPU cycles. Hope that all makes sense, & thanks for digging into it!

dustinfarris commented 7 years ago

Thanks for explaining why this was happening.

My project is ember-cashay and I'm just writing a bunch of tests to prove out basic CRUD operations—which I completed tonight! To move things forward, I put the delete button next to each project on the index component instead of having each detail component have its own delete action. This made the problem go away for now, so I'm going to close this.

I do think there is a bug here though, and I'm fairly certain I'm going to run into this again at which point I'll open a new targeted issue.

For instance, I tried doing a transitionToRoute after an updateProject mutation (instead of delete), and while I didn't see the same error as I expected I would, the data in the index component was not refreshed. I added an updateProject mutation handler to it and it was never executed. Something is up with the way Ember and Cashay lose track of each other during a transition.

dustinfarris commented 7 years ago

Reopening this:

Scenario A:

Scenario B:

I am seeing a lot of symptoms but having trouble triaging

dustinfarris commented 7 years ago

Without mutation handlers:

This is weird to me because the query for the other item is unrelated to the original item that i mutated to start.

dustinfarris commented 7 years ago

Going to reopen with a better description.

dustinfarris commented 7 years ago

See #157 for a better triage.