Netflix / falcor

A JavaScript library for efficient data fetching
http://netflix.github.io/falcor
Apache License 2.0
10.48k stars 446 forks source link

stop model / http-datasource retry on $error response from router #704

Open alexmuro opened 8 years ago

alexmuro commented 8 years ago

In my falcor-router instance I have two different routes to reference users

usersById[{integers:ids}]['fbId','first_name','last_name','email','birthday','profilePic','id']

which returns a bunch of information about a user or

usersByFbId[{keys:fbIds}]

which returns a reference to usersById.

There are instances where the client makes a request to usersByFbId[{keys:fbIds}]['profilePic','first_name''] where a user doesn't exist for that specific key. The router catches this no problem and returns an $error (i've also tried to have it return 'undefined' and 'null').

The problem comes when my client side Model keeps retrying the data source route until max retries is hit which can stop other legit routes from retrieving data. Is there a response I can give from my router that tells the model in this case that it just doesn't have any data and to stop making retry requests?

alexmuro commented 8 years ago

I found that Model has the errorSelector option for this kind of situation however the docs seem out of date as its seems that it gets the args errorSelector(paths,jsonGraphError) instead of errorSelector(jsonGraphError) and its difficult to tell what if its supposed to return anything. https://netflix.github.io/falcor/doc/Model.html#~errorSelector

after trying a bunch of variations on the one examples from the docs:

var model = new falcor.Model({
    source: new falcor.HttpDataSource('/model.json'),   
    errorSelector: function(error){
        error.$expires = -1000 * 60 * 2;
    }
});

I cannot get the model to stop retries on any given error.

It also looks like this is an area where the docs are not yet complete: http://netflix.github.io/falcor/documentation/model.html#error-handling

jhusain commented 8 years ago

Sorry the documentation is out of date. The error selector allows you to return a new error in place of the one being passed in. It seems like you're already doing the right thing to avoid endless retries (i.e. Returning an error from the Router). Any chance we can get a tight repo? Just go ahead and return an error from the Route all the time and confirm we get endless retries.

ThePrimeagen commented 8 years ago

Any update on this? Has @alexmuro produced some sort of working example? I can start by guessing my way into great success, but a hint would be great. We have examples that are returning errors from dataSources without said problems, so my guess its around the expiration time being added. If it is about the expiration time, then we some bugs that are creeping around that can be dangerous that we are planning on addressing very soon.

alexmuro commented 8 years ago

I am having a hard time consistently producing this issue but it does keep occurring for me in my app. I am using falcor with (https://github.com/ekosz/redux-falcor) and the order that components load the data sometimes creates the error and other times doesn't. I can't rule out the error is in of my use of falcor yet.

I can tell you for sure that it doesn't have anything to do with the $expires bugs, I am no longer using that in my errorSelector and I don't have any data that comes down from the server with $expires tags (altough its one of the next things I plan to investigate because it will be very useful for me).

I will make a separate repo to try and reproduce the error with only a model and a series of get requests today to see if I can do it. If I can reproduce in that context I will post here in the next day.

alexmuro commented 8 years ago

@michaelbpaulson below is a repo that reproduces the client side of my problem reliably by making 3 get requests each of which return an $error. It turns out you only need 2 of the get requests to produce the error but a single request will not produce it, which leads me to think it might have to do with .batch(), but .batch() is still magic to me so I'm not sure.

https://github.com/alexmuro/falcorTest

alexmuro commented 8 years ago

May have to do with batch and a combination of valid and $error returns from the same route with different keys/indices.

Or, if you only use the key 10204129826629813, which somehow magically gets changed to 10204129826629812 sometime between falcor.get and OnBeforeRequest, and so the initial request is never satisfied causing the max out on requests.

alexmuro commented 8 years ago

Per my previous comment that particular key gets transformed on line 68 of toPaths.js from 'falcor-path-utils'

isNumber(key) &&
                subPath.keys.push(parseInt(key, 10)) ||
                subPath.keys.push(key);

// resulting in
isNumber(10204129826629813) // true
parseInt(10204129826629813, 10) // 10204129826629812

this is because the highest number parseInt can handle correctly is apparently 9007199254740991..

I addded a check for val.length and val.length > 15 to isNumber and that fixed the problem for me. There might be a better way to check for that but the problem overall was the model sending a different request than the response it was expecting due to path mutation.

I am happy to close this thread, don't know if you want to change the code in the repo or not.

enderv commented 8 years ago

Didn't see this before I submitted my issue but I'm running into the same problem and would like to see it fixed if possible.

przeor commented 8 years ago

Same here. I am also looking around, posted an issue's comment here as well: https://github.com/Netflix/falcor/issues/284#issuecomment-220287940

jameslaneconkling commented 8 years ago

There is a PR that addresses this in falcor-path-utils. I forked the PR, repackaged falcor with the fork, and it fixed the problem. (see here)