hyper-react icon indicating copy to clipboard operation
hyper-react copied to clipboard

StateWrapper doesn't do set_state when using []=

Open zetachang opened this issue 7 years ago • 2 comments

Here is a falling test case

it 'is supposed to work' do
  k = Class.new {
    include React::Component
    before_mount do
      state[:foo] = 'bar'
    end

    def render
      div { state.foo }
    end
  }
  expect(k).to render('<div>bar</div>')
end

The current implementation only update the cached Hash of the backing state. I think this is a bug and could be fixed by actually invoking State.set_state.

/cc @catmando

zetachang avatar Oct 20 '16 06:10 zetachang

yeah I just noticed this as well... i think perhaps the intent at one time was to give a low level direct access to the underlying react state hash, and there are methods for doing that

There are a couple of competing issues here:

  1. @wied03 wants to separate out the non-react state extensions. These would go in a new gem that is now being tentatively called hyper-store.

  2. There is probably occasionally a need to reach down and just update the react.js state hash. This seems like a very infrequent operation, but should be documented as part of the DSL. Currently I think this was the intent of the [] and []= operators.

  3. An issue was just raised on gitter that the developer might like to reference the state name as a string. This might seem the same issue as 2, but the point is NOT to go around the reactrb goodness, but rather to just be able to use the name. I proposed that you can do this: state('my_state_name)to read the state, andstate!('my_state_name', optional_value)to update the state object or create an observer. This reads nicely i.e.state.fooandstate('foo')are the same whilestate.foo!andstate!('foo')` are the same.

the other approach might be to use the [] and ![] to access the reactrb state object by string an then just have another method (like native_state) to access the underlying state hash.

Note that using = for anything but the low level native state access is not a good idea since it leads to confusion for mutable objects. I.e. state.foo = 12 would lead you to believe that state.foo << 12 should also work. Hence we insist on state.foo! 12 and then state.foo! << 12 works.

catmando avatar Oct 20 '16 14:10 catmando

Just to make sure, the second point

There is probably occasionally a need to reach down and just update the react.js state hash.

Do you mean this.state.foo = "nextFoo" this kind of update, or this.setState({ foo: "nextFoo" }) ? The former one is discouraged by ReactJS doc given that it won't trigger any re-render.

zetachang avatar Oct 23 '16 12:10 zetachang