Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

caching failed remote fetches? #284

Open
bishtawi opened this issue Apr 22, 2015 · 21 comments
Open

caching failed remote fetches? #284

bishtawi opened this issue Apr 22, 2015 · 21 comments

Comments

@bishtawi
Copy link

I have a http source that makes a simple GET to my server to get some data into my store. If my server returns an error (404, 500, etc), I noticed that the store caches the error so any additional attempts to fetch the data auto fails (it doesnt even bother going back to the server). I would like to be able to rehit the server the next time my component attempts to fetch data from the store (on the off-chance that the server is back up).

@bishtawi
Copy link
Author

Just found the cacheError option in the fetch options of the store. When I set it to true false, the moment one of my components fetches data from the store, and the api request fails, I get an infinite number of api requests on my server. Seems like there is some kind of infinite loop going on? Ideally, I would like it to fail once and then retry only the next time a component fetches data from the source.

EDIT: I mean setting cacheError to false.

@jhollingworth
Copy link
Contributor

Hey, are you using Marty v0.8? We removed automatically converting http responses >= 400 in v0.9

@bishtawi
Copy link
Author

I am using Marty 0.9

@bishtawi
Copy link
Author

bishtawi commented May 2, 2015

Any ideas @jhollingworth ?

@jhollingworth
Copy link
Contributor

Hey, sorry been on holiday. I'm flying back today, will have a look at this properly when I get back

On 3 May 2015, at 12:48 am, Stephen [email protected] wrote:

Any ideas @jhollingworth ?


Reply to this email directly or view it on GitHub.

@bishtawi
Copy link
Author

bishtawi commented May 3, 2015

No worries. I appreciate the hard work you have put into this library.

@bishtawi
Copy link
Author

bishtawi commented May 3, 2015

Since Ive opened the bug, I switch to axios and same issue.

@jhollingworth
Copy link
Contributor

@bishtawi cacheError should be the solution to this problem. Not sure why its making lots of requests. Can you give me any more info about what requests its making?

Another thing to try is catching the error returned

this.fetch({
  locally() {
    ...
  },
  remotely() {
    return UserAPI.getUser(123).catch(function (error) {
      console.log('Failed to get user', error);
    });
  }
})

@bishtawi
Copy link
Author

bishtawi commented May 5, 2015

Maybe i am setting up my stores, queries or sources wrong. Here is what I have:

Store:

"use strict";
import Marty from 'marty'
import AreasQueries from '../queries/areasQueries'
import AreasConstants from '../constants/areasConstants'

let AreasStore = Marty.createStore({
  id: 'Areas',
  handlers: {
    addAreas: AreasConstants.RECEIVE_AREAS
  },
  getInitialState: function () {
    return {};
  },
  getAreas: function (type) {
    return this.fetch({
      id: type,
      locally: function () {
        return this.state[type];
      },
      remotely: function () {
        return AreasQueries.for(this).getAreas(type);
      }
    });
  },
  addAreas: function (type, areas) {
    this.state[type] = areas;
    this.hasChanged();
  }
});

export default AreasStore;

queries:

"use strict";
var Marty = require('marty');
var AreasAPI = require('../sources/areasAPI')
var AreasConstants = require('../constants/areasConstants')

var AreasQueries = Marty.createQueries({
  id: 'AreasQueries',
  getAreas: function (type) {
    return AreasAPI.for(this).getAreas(type).then(function (res) {
      this.dispatch(AreasConstants.RECEIVE_AREAS, type, res.data);
    }.bind(this));
  }
});

module.exports = AreasQueries;

sources:

"use strict";
import Marty from 'marty'
import axios from 'axios'

let AreasHttpAPI = Marty.createStateSource({
  type: 'http',
  id: 'AreasHttpAPI',
  getAreas: function (type) {
    return axios.get('api/' + type);
  }
});

export default AreasHttpAPI;

Using axios shouldnt matter as I have had this bug back when I was using fetch.

The variable type that I am passing around is just a string.

@bishtawi
Copy link
Author

bishtawi commented May 5, 2015

Setting cacheError to false results in a never ending stream of requests to my server. It just keeps sending requests. I have to exit the page to stop it. I can see the requests being made using chrome dev tools.

EDIT: I should say that it only keeps sending requests when my server is down. If the server is up, it doesnt continuously send requests.

I am using Marty 0.9.11.

@jhollingworth
Copy link
Contributor

Your code looks fine, this is probably due to an action being dispatched after the fetch failed which causes the component to re-start the fetch and thus creating the loop.

If you have a chance could you debug in devtools and see what action is causing the component to re-render?

@bishtawi
Copy link
Author

bishtawi commented May 5, 2015

I am about to go to bed, so I'll do some more debugging tomorrow. In the mean time, heres my container:

module.exports = Marty.createContainer(Areas, {
  listenTo: AreasStore,
  fetch: {
    areas() {
      return AreasStore.for(this).getAreas(this.props.type);
    }
  },
  pending() {
    return <div className='container pending'>Loading {this.props.type}...</div>;
  },
  failed(errors) {
    return <div className='container failed'>Failed to load {this.props.type}</div>;
  }
});

@bishtawi
Copy link
Author

bishtawi commented May 6, 2015

Alright, I loaded up the marty devtools and I cant seem to figure out what action or component is causing the rerender. I took a screen shot:
screen shot 2015-05-05 at 9 53 37 pm

@jhollingworth
Copy link
Contributor

Interesting, can you try commenting out this block of code and see if that makes a difference? (You might need to re-build if you aren't using browserify)

@bishtawi
Copy link
Author

bishtawi commented May 7, 2015

Nope, does not make a difference.

@finnsch
Copy link

finnsch commented May 19, 2015

Hey,

I had the exact same problem as OP, but I managed to find the cause of the problem in the Marty source files. I am not sure if this solution is perfect or anything, but it fixed the problem for me. Check out the highlighted line in the link. It caused my container to run the fetch method of my store in an endless loop, because it emitted a store change on every failed fetch. In order to prevent this from happening, I've added a short if statement to check if the cacheError option was set to false and killed the execution of the hasChanged method.

What do you think of this solution? I am not 100% sure that it doesn't break in other use cases. :/

Thanks for your awesome library btw. ;)

@bishtawi
Copy link
Author

bishtawi commented Jun 3, 2015

@jhollingworth What do you think of Pytal's solution?

@quazzie
Copy link
Contributor

quazzie commented Jun 26, 2015

Got the same problem, infinite requests if cacheError is false.
I went around it by having cacheError as default and then in an action i clear store.__failedFetches.

´´´
class Store extends Marty.Store {
@Handles(Const.ACTION_REFETCH)
onRefetch() {
this.__failedFetches = {};
this.hasChanged();
}
}
´´´

@taion
Copy link
Member

taion commented Jun 26, 2015

Updated link for #284 (comment): https://github.com/martyjs/marty/blob/v0.9.17/lib/store/fetch.js#L95, or https://github.com/martyjs/marty-lib/blob/master/src/store/storeFetch.js#L94 for v10.

That hasChanged is necessary in general to get the container to update and pass along the "not found" value.

The thing is - the store has to have a value to pass down to the container. If you're not caching errors, and the request fails, what value is the container supposed to receive? It has to be fed something.

@taion
Copy link
Member

taion commented Jun 26, 2015

I guess the right thing to do here would be to change the behavior of cacheError such that we still do cache the error, but then clear the cache once the error is read. Right now, the follow-up attempt at a local read won't see the error if we don't cache it.

@anthonator
Copy link

I'm seeing this in v0.10. Could be related to this.

#304

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants