mattkrick / cashay

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

Cache went stale and wasn't recreated #157

Open dustinfarris opened 7 years ago

dustinfarris commented 7 years ago

I'm not sure where this is breaking down either in Cashay or in my own code; but here's the reproducible scenario:

Note: I typed this up by hand (no copy+paste) so if there's some stupid syntax error its probably just a typo in this GH issue alone.

The SETUP

Two routes:

Starting with the second route: a detail page that can create and update maps. Creating is done by clicking "NEW" on the index page, which triggers an action that generates a UUID, sets a isCreatingNewMap flag in redux state, and redirects to the detail page.

The detail page has two queries and two mutations.

The two mutations are createMap and updateMap

The queries are identical except one has @cached and is used when the map is being created. This is so the createMap mutation knows to grab subfields.

This is the relevant piece of the MapDetail component:

/**
 * map detail component
 */

const cachedMapQuery = `
{
  map @cached(id: $map_id, type: "Map") {
    id
    name
  }
}
`;

const mapQuery = `
{
  map (id: $map_id) {
    id
    name
  }
}
`;

const mutationHandlers = {
  createMap(optimistic, response, current) {
    if (optimistic) {
      Object.assign(current.map, optimistic.new_map.map_details);
      return current;
    }
  },
  updateMap(optimistic, response, current) {
    if (optimistic) {
      Object.assign(current.map, optimistic.map_details);
      return current;
    }
  }
};

const stateToComputed = (state, attrs) => {  // `attrs` is like `props` in React

  const query = state.isCreatingNewMap ? cachedMapQuery : mapQuery;

  const { data: { map } } = cashay.query(query, {
    op: state.isCreatingNewMap ? 'map-detail-cached' : 'map-detail',
    key: attrs.map_id,
    variables: { map_id: attrs.map_id },
    mutationHandlers
  });

  return { map, isCreatingNewMap: state.isCreatingNewMap };
};

const MapDetail = Component.extend({

  isNewMap: oneWay('isCreatingNewMap'),  // this allows me get/set without modifying the parent

  saveMap: task(function *(changeset) {
    const map_id = this.get('map_id');  // from `attrs`
    let ops;
    if (this.get('isCreatingNewMap')) {
      ops = {
        'map-detail-cached': map_id,
        'maps-index': true,
      };
    } else {
      ops = {
        'map-detail': map_id,
        'maps-index': true,
      };
    }

    let promise;

    if (this.get('isNewMap')) {
      this.set('isNewMap', false);
      promise = cashay.mutate('createMap', {
        ops,
        variables: {
          new_map: {
            id: map_id,
            map_details: changeset.get('change')
          }
        }
      });
    } else {
      promise = cashay.mutate('updateMap', {
        ops,
        variables: {
          id: map_id,
          map_details: changeset.get('change')
        }
      });
    }

    yield promise.then(response => {
      if (response.error) {
        this.set('status', 'error');
      } else {
        this.set('status', 'saved');
      }
    });
  })
});

Back to the index page, this is the relevant piece of the MapsIndex component:

/**
 * maps index component
 */

const mapsQuery = `
{
  maps {
    id
    name
  }
}
`;

const mutationHandlers = {
  createMap(optimistic, response, current) {
    if (optimistic) {
      current.maps.push({
        id: optimistic.new_map.id,
        ...optimistic.new_map.map_details,
      });
      return current;
    }
  },
  updateMap(optimistic, response, current) {
    if (optimistic) {
      // I can't just Object.assign here because Ember observes everything
      // and gets cranky if you mutate without using its special helpers.. this 
      // behavior is going away eventually—I hope.
      const map = current.maps.findBy('id', optimistic.id);
      const index = current.maps.indexOf(map);
      current.maps.splice(index, 1, {
        ...map,
        ...optimistic.map_details
      });
      return current;
    }
  }
};

const stateToComputed = () => {

  const { data: { maps } } = cashay.query(mapsQuery, {
    op: 'maps-index',
    mutationHandlers
  });

  return { maps };
};

const MapsIndex = Component.extend({
});

The SCENARIO

Part 1 (The happy version)

Step 1 - User visits index

Cashay fires a server query:

{
  maps {
    id
    name
  }
}

All maps are displayed with their name

Step 2 - User clicks "NEW"

And is taken to a detail page with a freshly minted UUID in the URL

No server request. Cashay executes cachedMapQuery shown in the component code above.

Step 3 - User fills in name, triggers Save

Cashay executes a mutation:

map-detail optimistic mutation handler fires! maps-index optimistic mutation handler fires!

then sends a server request

mutation ($new_map: NewMapInput!) {
  createMap(new_map: $new_map) {
    name
    id
  }
}

Step 4 - User navigates back to index

As part of this transition away, the detail route dispatches to turn off isCreatingNewMap. This triggers a redux subscribe callback. The MapDetail component, which has not been torn down yet, runs stateToComputed one more time which triggers a cashay.query, but this time using mapQuery instead of cachedMapQuery (shown in the component code above). This triggers a (unnecessary?) server request:

query ($map_id: ID!) {
  map(id: $map_id) {
    id
    name
  }
}

The transition completes and the user is now on the index page. The new map appears under the rest with its name. Everyone is happy so far.

Step 5 - User clicks the new map going back to its detail page

As in step 4, the map is looked up using mapQuery (the one without @cached). No new server queries—presumably because of the side-effect query in step 4, Cashay must be using the cache from that query.

Step 6 - User edits the name of the map and triggers save

map-detail optimistic mutation handler fires! maps-index optimistic mutation handler fires!

And Cashay kicks off a mutation request to the server:

mutation ($id: ID!, $map_details: MapDetailsInput!) {
  updateMap(id: $id, map_details: $map_details) {
    name
    id
  }
}

Step 7 - User navigates back to the index page

No new server queries. The maps new name appears as it should.


Part 2 (The sad version)

The difference here is we'll be editing an existing map's name, instead of creating a new map and then editing it.

Step 1 - User visits index page

Cashay fires a server query:

{
  maps {
    id
    name
  }
}

All maps are displayed with their name

Step 2 - User clicks an existing map and taken to detail page

Cashay executes mapQuery in the component as shown in the component code above:

query ($map_id: ID!) {
  map(id: $map_id) {
    id
    name
  }
}

Step 3 - User edits the map's name and triggers Save

map-detail optimistic mutation handler fires! maps-index optimistic mutation handler throws error!

dustinfarris commented 7 years ago

The error happens when I leave the index page and on the new page cashay queries new data from the server.

dustinfarris commented 7 years ago

Ok, starting to understand:

What I'm still struggling with is how, if the maps-index response is flushed, does the maps-index component continue to render. For example if I:

dustinfarris commented 7 years ago

Getting closer (without mutation handlers this time):

When the user visits maps index, cashay stores the result and entities in redux, e.g.:

{
  cashay: {
    entities: {
      Map: {
        1: { id: "1", name: "First Map" }
        2: { id: "2", name: "Second Map" }
        3: { id: "3", name: "Third Map" }
      }
    }
    ops: {
      maps-index: { variables: {}, status: "complete" .. }
    }
    result: {
      maps: [
        "Map::1"
        "Map::2"
        "Map::3"
      ]
    }
  }
}

cashay also caches a "response" object in memory, which, as far as i can tell at this point, is the denormalized result of the query, be it from the server, or from redux (or both i suppose in some cases).

So long as this "response" object exists, cashay will not ask redux, much less the server for an update. This is why mutationHandlers are necessary when there is a "response" object" in memory for a particular query.


When my user clicks a specific Map, they are taken to a detail page that executes a new "map-detail" query. As part of this operation, cashay flushes the above-mentioned response object for the maps-index operation. Then let's say the user executes an update mutation to change the name of the map. Cashay updates redux so it looks something like:

    entities: {
      Map: {
        1: { id: "1", name: "CHANGED NAME!" }
        ...

When the user navigates back to the maps index page, cashay flushes the response for the map-detail query. Cashay also sees that it no longer has a response object for the maps-index query. So it presumably checks redux for an existing result. And it finds one! So it pieces together a new response, but entirely from local state (redux). Because cashay updated entities during the mutation, the new response for the maps-index query contains the updated name. All is good.


Here's where it breaks down

Then the user clicks the same map and is taken back to the detail page. Cashay sees no "response" object for map-detail (it was flushed), but is able to piece together a new response from redux. Because no server query is required, the maps-index response object is not flushed!

The user changes the name again. Cashay executes a mutation.

The user navigates back to the index and doesn't see the change!

This is because cashay found that the response object for maps-index was still there and used that instead of checking redux.

How do we force flush a response when the user leaves the maps-index page but has not triggered a new server query?


Another way it breaks down

Then the user access yet another map which again flushes the response for the maps-index query.

Then, the user clicks a big 'ol PLUS icon in the navbar to add a new map.

Then the user fills out a new map name and mutation is executed. There are now 4 maps on the server.

The user navigates back to the maps index page, but sees just 3 maps!

This is because cashay, again, saw that there was no longer a denormalized response for the maps query, but there is still a result entry for the query. But that value (still) looks like this:

    result: {
      maps: [
        "Map::1"
        "Map::2"
        "Map::3"
      ]

Still 3 maps.

How do we update the result for "maps"? Can it be done without re-triggering a server request when we already have all the data we need?


@mattkrick i think this issue is ready for your eyes. This last comment I think summarizes the problem pretty well, but see above for more context.

dustinfarris commented 7 years ago

Side note: :point_up: I suspect that this problem goes away when @live is introduced. But I'm not quite there yet with my server (Elixir/Phoenix) so I'm stuck with the stale result data shown above for now.

mattkrick commented 7 years ago

there's a lot to dig into & i'm not sure i grok it all, but if stale data is getting to the view layer, then it has more to do with the ember integration i think. you shouldn't ever have to manually flush a result because the result will change whenever a mutation occurs as long as there is a mutation handler.

dustinfarris commented 7 years ago

tl;dr,

This is how everything works great with a mutation handler:

Continuing, this is how it can break down:

Sent from my iPhone

On Jan 11, 2017, at 5:01 AM, Matt Krick notifications@github.com wrote:

there's a lot to dig into & i'm not sure i grok it all, but if stale data is getting to the view layer, then it has more to do with the ember integration i think. you shouldn't ever have to manually flush a result because the result will change whenever a mutation occurs as long as there is a mutation handler.

— 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

@mattkrick there may very well be some nuance at play with the Ember view layer, but I think there is a real bug here, and I think the reason you may not have run across it as much or at all in the action project is because your use of @live circumvents your need for mutationhandlers in this kind of scenario.