reflux / refluxjs

A simple library for uni-directional dataflow application architecture with React extensions inspired by Flux
BSD 3-Clause "New" or "Revised" License
5.36k stars 330 forks source link

listenAndPromise doesn't return data? #352

Open willdady opened 9 years ago

willdady commented 9 years ago

I'm just a little confused about using promises with reflux. I'm using qwest to query my endpoint as shown below, the onGetResourceCompleted handler fires as expected but data is undefined. Why is that?

var Actions = Reflux.createActions({
  'getResource': { asyncResult: true }
});

Actions.getResource.listenAndPromise(function (id) {
  return qwest.get('http://localhost:8000/my-data/'+id, null, { withCredentials: true });
});

var ServiceStore = Reflux.createStore({

  listenables: Actions,

  init: function () {
    Actions.getResource('');
  },

  onGetResourceCompleted: function (data) {
    console.log('OK', data); // Get's called but data is undefined. Why?
  }

});

I can see the data loads correctly by looking at dev tools as well as calling qwest in isolation by simply doing:

qwest.get('http://localhost:8000/my-data/'+id, null, { withCredentials: true }).then(function(data) {
  console.log('OK', data);
});
LongLiveCHIEF commented 9 years ago

Can't really help you with this one here, it's likely not even a Reflux usage issue. Please refer to help channels for this topic:

For questions please use the following communication channels:

  1. StackOverflow with the refluxjs tag
  2. #reflux channel on Reactiflux Slack group. Sign up here for an account.
  3. Gitter

Please only use the issue tracker for bugs and feature requests only.

willdady commented 9 years ago

I've asked the question on StackOverflow here.

I don't really get why this issue was closed. How is what I've described not a bug?

LongLiveCHIEF commented 9 years ago

because you have code errors. (You're not doing anything with the promise.)

willdady commented 9 years ago

Yeah I don't know what you mean by 'You're not doing anything with the promise'. It's being returned from listenAndPromise as documented in the ReadMe.

listenAndPromise - Expects a function that returns a promise object, which is called when the action is triggered, after which promise is called with the returned promise object.

Which is what I'm doing here. The 'completed' listener fires with undefined where the loaded data is expected.

Actions.getResource.listenAndPromise(function (id) {
  return qwest.get('http://localhost:8000/my-data/'+id, null, { withCredentials: true });
});

The following works as expected. The 'completed' listener is called with the loaded data:

Actions.getResource.listen(function (id) {
  Actions.getResource.promise(
    qwest.get('http://localhost:8000/my-data/'+id, null, { withCredentials: true })
  );
});

My understanding of the docs is that the two snippets above should give the same result but they don't.

spoike commented 9 years ago

Maybe a bug?

akre54 commented 9 years ago

My fetch function looks something like this:

fetch(id, params) {
  let method = 'GET';
  let url = getUrl(id, params);

  return sync(url, {method, body: params})
    .then(Actions.fetch.completed)
    .catch(Actions.fetch.failed);
},

This calls Store.onFetchcompleted with my response, which is great, but I can't chain anything off of my fetch (i.e. within the store) because triggerPromise doesn't resolve with anything. It'd be great if it actually passed back my data (i.e. args[0]) in that resolve call.

I ended up just calling Actions.fetch instead of this.fetch, but it seems to me Reflux shouldn't care which one you use.

// in store
Actions.fetch().then((resp) => { ... }) // works
this.fetch().then((resp) => { ... }) // doesn't (resp is undefined)
willdady commented 9 years ago

If it's any help I can get listenAndPromise working with a Qwest promise only if I wrap it in a Q promise like so:

function wrapQwest(promise) {
  var deferred = Q.defer();
  promise.then(function (data) {
    deferred.resolve(data);
  }).catch(function (error, response) {
    deferred.reject({
      status: this.status,
      response: response
    });
  });
  return deferred.promise;
}

...

Actions.someAction.listenAndPromise(function (params) {
  return wrapQwest(qwest.get(url, params));
});

So it seems reflux doesn't work with Qwest's promise implementation when used with listenAndPromise though it works fine with promise as stated above.

LongLiveCHIEF commented 9 years ago

I've looked into this, and despite my initial thoughts that caused me to close this, I've now determined that this is actually not a bug, because I'm not sure it can be fixed. This is actually a design flaw that is creating a race condition. I started to answer @willdady's SO question, and realized that my answer was actually a "non-answer", so I thought I'd bring it back to this issue.

Explanation

In the example given MyStore.init function initiates execution and the resolve/reject of the promise, but it doesn't actually do anything with the promise after that.

I think that's what @willdady intended, but I think where the "flaw" comes in, is how the async action handler works for the reflux completed child action shortcut. The shortcut will call the completed handler onGetResourceCompleted whenever the getResource method is called and finished executing.

The problem is that the reference of getResource is executed immediately, but it's the value that's asynchronous not the promise reference itself. In order to resolve his promise and return the value to a callback, he'd need to do this:

init: function(){
    return (function(){
      return Actions.getResource('').then(function(data){
        return data;
      })
  })
}

Since onGetResourceCompleted executes immediately after getResource promise is executed, it's very unlikely the expected return value of the promise will have been assigned in time. The other problem, is that in order to delay the execution of the handler's callback, the handler needs to control invocation of the promise, meaning the promise would have to be returned as a deferred, instead of

In this case, the completed shortcut is redundant to the purpose of promises themselves, and you'd be better off removing the onGetResourceCompleted method entirely, and just executing Actions.getResource.then().catch() inside init would achieve the desired outcome without needing the Completed shortcut and listenAndPromise combo.

It's all very "inception"-ish, and I recommend some sort of adult beverage to ease the brain pain when you walk through this.

@willdady I'm sorry I didn't give this question the respect it deserved on the first go-around.

@spoike I have some thoughts on this, and I think they directly relate to our most recent roadmap discussion. I think the shortcuts will wind up being deprecated, and the listen/promise might be handled slightly differently.

Going to tag this as 0.3.0.

jfairley commented 9 years ago

If I'm reading this correctly, I believe I'm having the same issue in a different place. As of 0.2.10, functions built with listenAndPromise no longer return a promise. In 0.2.9 they did.

Here's my code...

// ClipActions.js
const ClipActions = Reflux.createActions({
    load: {asyncResult: true}
});
ClipActions.load.listenAndPromise(recordId =>
        request.get(`/records/${recordId}/clips`)
            .type('application/json')
            .then(res => res.body)
            .catch(err => {
                AlertActions.error(err.message, err.res.text);
                throw err;
            })
);

// ClipActions_test.js
        it('should load clips', () => {
            let response = [{}, {}];

            let request = API
                .get(`/records/5/clips`)
                .reply(200, response);

            return ClipActions.load(5)
                .then(data => {
                    expect(data).to.deep.equal(response);
                    request.done();
                });
        });

With v0.2.9 and prior, this test passes.

With v0.2.10 and v0.2.11, I get the following error, because load() no longer returns a promise.

1) ClipActions load() should load clips:
     TypeError: Cannot call method 'then' of undefined
LongLiveCHIEF commented 9 years ago

@spoike here's that explanation I was searching for. (see my comment above)

spoike commented 9 years ago

I've been meaning to move this issue to reflux-promise, since reflux and reflux-core doesn't have the promises in it's API any longer.

LongLiveCHIEF commented 9 years ago

The problem I described though, has nothing to do with promises, it has to do with reflux-core and the design of action handlers.

spoike commented 9 years ago

@H3Chief I was mostly referring to @jfairley's comment and the actual facing issue which is a bug that should be adressed in reflux-promise.

The child actions issue should be worked on as well, and if that means changing the Reflux interface a bit, then that is okay with me as long as we follow semantic versioning for breaking changes.

jfairley commented 9 years ago

FYI, we recently upgraded to reflux-core:0.3.0 and reflux-promise:1.0.1, and actions again return promises. This is true for .listen as well as .listenAndPromse. I just assumed this issue was fixed and not yet closed.

To illustrate (snipped down for brevity):

actions:

PlayerActions.updateClip.listen(detail => {
    try {
        // ... do stuff
        PlayerActions.updateClip.completed();
    } catch (err) {
        PlayerActions.updateClip.failed(err);
    }
});

PlayerActions.checkHls.listenAndPromise(srcPath => request.get(srcPath).then(res => res.body));

tests

it('should ...', () => {
    return PlayerActions.updateClip({ ... })
        .then(res => {
            expect( ... ).to.equal( ... );
        });
});

it('should ...', () => {
    return PlayerActions.checkHls('http://...')
        .then(res => {
            expect(res).to.deep.equal({ ... });
        });
});
devinivy commented 8 years ago

Any official word on this? My impression is that this works, but it's unclear given the changelogs, etc.