FormidableLabs / freactal

Clean and robust state management for React and React-like libs.
MIT License
1.65k stars 46 forks source link

Using async functions for effects #56

Closed chipit24 closed 7 years ago

chipit24 commented 7 years ago

I have the following effects:

const setLoading = softUpdate(({ loading }, val) => ({ loading: loading += val ? 1 : -1 }));

const getLayouts = efects => (async () => {
  await effects.setLoading(true);
  const {data: layouts} = await axios.get('/layouts');
  await effects.setLoading(false);

  return state => ({ ...state, layouts });
})();

const effects = {
  getLayouts,
  setLoading
};

export default effects;

However, the await effects.setLoading call does not seem to actually trigger the setLoading effect.

Is there a recommended way I can use async functions for effects?

divmain commented 7 years ago

Using async functions should work fine - it's all promises under the hood. I do see that you've misspelled efects in the snippet you've shared. I'm also unsure why you invoke the getLayouts inner function (it is wrapped and invoked as an IIFE). That doesn't look correct either. My guess is that, should you fix both of these, getLayouts will work fine.

chipit24 commented 7 years ago

Ah, good eye, the typo was causing issues for that effect.

Correct me if I'm wrong, but an effect should return a function that takes in the current state, or a promise that resolves to such. An async function declaration doesn't fit that definition, however, if you were to call an async function, it would (hence the IIFE).

Looking at the source (hocState.setState(reducer(hocState.state))), if I were to omit the IIFE, setState would receive a promise.

I ended up forking the repo and adding extra test cases for effects. Using an async function failed unless it was wrapped in a IIFE (in which case it would be the same as using a promise, nothing special). I didn't end up with an elegant solution for using async functions as you described, but I can re-visit it.

divmain commented 7 years ago

It's a bit more nuanced than that. If the effect function returns a promise (which an async function would), the reducer is not run until that promise resolves.

If you were writing tests for effects that weren't working correctly, it may have been related to a small bug in how effects resolved. If that's not the case, I'd love to see the tests that you've written. In the meantime, I've added a test for async function effects here.

chipit24 commented 7 years ago

I checkout out the latest master and ran yarn install && npm run build && npm run test, but now things are failing with:

nested-state-injection.spec.js:63
  it("children are updated when intermediate state injections are present", async function () {
                                                                            ^^^^^
SyntaxError: missing ) after argument list

I didn't dig into it, so I'm not sure if my tests would still fail (I assume they would), but this is one I wrote for a quick test:

it("resolves to functions which update state", () => {
  const NUM_STATE_FUNCS = 4;
  const state = {};
  const hocState = {
    setState: sinon.spy(),
    state
  };
  const updateState = () => state;
  const effects = getEffects(hocState, {
    effectA: () => updateState,
    effectB: () => Promise.resolve(updateState),
    effectC: () => new Promise(resolve => resolve(updateState)),
    effectD: () => async () => updateState,
    effectE: () => {}
  });

  return Promise.all(
    Object.values(effects).map(effect => effect())
  ).then(() => {
    expect(hocState.setState).to.have.callCount(NUM_STATE_FUNCS);
    expect(hocState.setState).to.have.always.been.calledWith(state);
  });
});

Looking at your test, I see what I did wrong. effectD: () => async () => updateState should just be effectD: async () => updateState and things would work as expected. Don't know why this didn't occur to me before 🙈 thanks!