graphistry / falcor

Graphistry forks of the FalcorJS family of projects in a lerna-powered mono-repo.
23 stars 2 forks source link

Inconsistencies in Error reporting between recycleJSON true and false #16

Open jameslaneconkling opened 7 years ago

jameslaneconkling commented 7 years ago
const model = new Model({
  source: {
    get: () => Observable.timer(100)
      .concat(Observable.throw({
        status: 500
      }))
  },
  recycleJSON
})

model.get(['items', 0, 'title'])  
  .subscribe({
    error(err) {
      console.log('Error', err)
    }
  });

If recycleJSON is true, logs:

// > { $type: 'error', value: { status: 500 } }

If recycleJSON is false, logs:

// > [{ "path": ["items", 0, "title"], "value": { "status":500 } }]
jameslaneconkling commented 7 years ago

actually, looks like recyleJSON: true just sets treatErrorsAsValues, which is fine by me. However, another inconsistency arises:

const model = new Model({
  source: {
    get: () => Observable.timer(100)
      .concat(Observable.throw({
        status: 500
      }))
  },
  recycleJSON
})
  .treatErrorsAsValues();

model.get(['items', 0, 'title'])  
  .subscribe({
    next(data) {
      console.log('Data', data);
    }
    error(err) {
      console.log('Error', err);
    }
  });

If recycleJSON is true, logs:

// > Error: { $type: 'error', value: { status: 500 } }

If recycleJSON is false, logs:

// > Data: { "items": {"0": {"title": { "status": 500 } } } }
// > Error: { $type: 'error', value: { status: 500 } }

And if run against Netflix's model, logs:

// > Data: { "items": {"0": {"title": { "status": 500 } } } }

I'm not 100% sure what the desirable behavior is. Probably makes sense for behavior to be consistent between recycleJSON true/false. Not sure whether or not it also makes sense to track behavior in Netflix's model as well...

trxcllnt commented 7 years ago

@jameslaneconkling you're right, the recycleJSON version should be onNext'ing value at minimum. Will look into it now.