FormidableLabs / freactal

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

State Exposed to Effects? #29

Open zebulonj opened 7 years ago

zebulonj commented 7 years ago

Is there a way to read state in effects, prior to the atomic state transformation?

In redux, a prototypical example would be:

export function fetchPostsIfNeeded(subreddit) {
  // Note that the function also receives getState()
  // which lets you choose what to dispatch next.

  // This is useful for avoiding a network request if
  // a cached value is already available.

  return (dispatch, getState) => {
    if (shouldFetchPosts(getState(), subreddit)) {
      // Dispatch a thunk from thunk!
      return dispatch(fetchPosts(subreddit))
    } else {
      // Let the calling code know there's nothing to wait for.
      return Promise.resolve()
    }
  }
}

In that example, getState() is used to inspect whether a fetch is needed.

VinSpee commented 7 years ago

I think something like this would work:

const wrapComponentWithState = provideState({
  effects: {
    shouldFetchPosts: (effects, subreddit, state) => {
      // ...your logic
    },
    fetchPosts: (effects, subreddit) => {
      // your logic
    },
    fetchPostsIfNeeded: (effects, subreddit) => state => {
      if (effects.shouldFetchPosts(state, subreddit)) { // you have the option of passing state here, or you could access it inside of your `fetchPosts` effect, depending on your needs
        effects.fetchPosts(subreddit);
      }
      return Promise.resolve();
   },
});

notice that effects return a function that is passed state as a parameter. See the effect arguments for details.

zebulonj commented 7 years ago

Yeah. It's clear that arguments can be passed to the effects, and that state—exposed to the calling component via injectState()—could be one of those arguments. I'd prefer to hide some of that from the consumer of the state.

What I'm describing would probably have to be accomplished via middleware, same as Redux requires redux-thunk middleware to expose getState() to the action creator.

zebulonj commented 7 years ago

@VinSpee I just looked more closely at your proposed approach, and see now that it doesn't require the consumer to pass the state. Sorry for overlooking that.

A concern...

In your proposed fetchPostsIfNeeded, the following portion is the reducer:

const reducer = state => {
  if (effects.shouldFetchPosts(state, subreddit)) { // you have the option of passing state here, or you could access it inside of your `fetchPosts` effect, depending on your needs
    effects.fetchPosts(subreddit);
  }
  return Promise.resolve();
}

However, freactal expects the reducer to a synchronous function that returns a transformed state. Effects have to take the form:

const effect = ( effects, ...args ) => Promise.resolve( state => transform( state ) );

The Promise.resolve( ... ) wrapping the synchronous state transformation can be implied, because of the way that effects are wrapped when the provider is created, but ultimately the reducer where state is received and transformed must be synchronous.

The Promise you are returning in your proposed effect would become the argument passed to hocState.setState( ... ) via getEffects and the enclosed applyReducer().

divmain commented 7 years ago

Confession: I originally passed both effects and state to each effect upon invocation, with the same thoughts you've expressed here.

However, I ultimately removed it since the promise-that-resolves-to-a-function thing is unfamiliar to most folks, and providing state both at the top of the effect and as an argument to the resolved-to-function could get confusing quite fast.

This might be an appropriate place for a helper function of some sort - something that makes it easier to build conditional effects. Let me think on this a bit. And if you have any thoughts on the matter, I'm all ears. For now, there's a sufficient work-around in passing in state (or a piece of state) into the effect directly... at least it is explicit :)

markacola commented 7 years ago

I have been using this helper.

const pure = (fn) => (effects, ...args) => state => (fn(effects, state, ...args), state)

I don't know that the naming is the greatest, but happy to submit a quick pull request to add a helper.

divmain commented 7 years ago

@markacola, a PR would be great. I think we'll want a different name though, since "pure" is quite the overloaded term at this point.

I assume you're using this pure function to wrap each effect? The problem here is that the effect might return a promise, and the state-update function needs to be synchronous.

Another possibility would be to add this functionality as a middleware. It would wrap each effect and add an additional state arg before the rest, much like you've done with the helper above. I wouldn't have a problem with shipping this as a standard baked-in middleware.

markacola commented 7 years ago

Yes, I wrap each effect with this pure function. This does return a synchronous state-update function, and ignores whatever is returned from the fn, returning the original state instead. The idea is that the effect can only affect the state by invoking other effects, so it is pure in that sense, but I cannot think of another name. A middleware could be very clean. Would you want the middleware exposed from helpers.js?

cheapsteak commented 6 years ago

Another use case for wanting access to state along with effects - We need to read the state to make an ajax request, then modify the state with values from the response

divmain commented 6 years ago

Here's what I'm thinking for the middleware. For the time being, this should serve as a work-around for anyone here still wondering how to do this:

import React, { Component } from "react";
import { render } from "react-dom";
import { provideState, injectState } from "freactal";

// THIS IS THE MIDDLEWARE
const supplyStateToEffects = freactalCxt => Object.assign({}, freactalCxt, {
  effects: Object.keys(freactalCxt.effects).reduce((memo, key) => {
    memo[key] = (...args) => freactalCxt.effects[key](freactalCxt.state, ...args);
    return memo;
  }, {})
});

const wrapComponentWithState = provideState({
  initialState: () => ({ counter: 0 }),
  middleware: [supplyStateToEffects],
  effects: {
    // If the counter is at 10, stop increasing the counter.
    addOneUpToTen: (effects, suppliedState, text) => {
      if (suppliedState.counter > 9) { return null; }
      return state => Object.assign({}, state, { counter: state.counter + 1 })
    }
  }
});

const Parent = wrapComponentWithState(injectState(({ state, effects }) => (
  <div>
    {`Our counter is at: ${state.counter}`}
    <button onClick={effects.addOneUpToTen}>Add One</button>
  </div>
)));

render(<Parent />, document.getElementById("root"));

You can see this example in action here.

Keep in mind that the state passed into the effect is the computed state; it'll include state from all parent state containers.

@zebulonj @VinSpee @cheapsteak - does this address your need, and would it be helpful to include middleware like this along with the rest of freactal?

ksvitkovsky commented 6 years ago

To anyone trying to solve this challenge, note that when you use the middleware given above, state is not being passed to effects that are invoked via effects-argument. So you should either pass it explicitly or not expect it to be passed in those "inner" effects.

IMO, it would be better to enforce passing state to invoked effects in your project for consistency

effects: {
  publicEffect: (effects, state, ...args) {
    effects.privateEffect(state, arg);
  },
  privateEffect: (effects, state, ...args) {}
}

@divmain I think the issue we discuss here is quite common and should at least be covered in README. Middleware could be handy as well (note the behaviour I described above) and changing API for effects to expect second argument to be state is even better

bingomanatee commented 6 years ago

This is doable, in two stages; you turn the state modifier into an identity function that also calls a second effect with parameters from state. Check out side effects in freactal-seed.

Sent from my iPhone

On Nov 28, 2017, at 1:13 AM, Dale Bustad notifications@github.com wrote:

Here's what I'm thinking for the middleware. For the time being, this should serve as a work-around for anyone here still wondering how to do this:

import React, { Component } from "react"; import { render } from "react-dom"; import { provideState, injectState } from "freactal";

const supplyStateToEffects = freactalCxt => Object.assign({}, freactalCxt, { effects: Object.keys(freactalCxt.effects).reduce((memo, key) => { memo[key] = (...args) => freactalCxt.effects[key](freactalCxt.state, ...args); return memo; }, {}) });

const wrapComponentWithState = provideState({ initialState: () => ({ counter: 0 }), middleware: [supplyStateToEffects], effects: { addOneUpToTen: (effects, injectedState, text) => { if (injectedState.counter > 9) { return null; } return state => Object.assign({}, state, { counter: state.counter + 1 }) } } });

const Parent = wrapComponentWithState(injectState(({ state, effects }) => (

{`Our counter is at: ${state.counter}`}

)));

render(, document.getElementById("root")); Keep in mind that the state passed into the effect is the computed state; it'll include state from all parent state containers.

@zebulonj @VinSpee @cheapsteak - does this address your need, and would it be helpful to include middleware like this along with the rest of freactal?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.