Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Add class-like state behavior option to withState #357

Closed
wants to merge 1 commit into from

Conversation

timkindberg
Copy link
Contributor

Adds an additional optional param set for withState, where if you only provide initialState, then stateName and stateUpdaterName are defaulted to 'state' and 'setState'. In this form setState works exactly like setState from a typical class component (because its really just proxying straight to this.setState).

Example Usage:

import { withState } from 'recompose';

// setting initial state only
const enhance = withState({ red: 0, green: 0, blue: 0 });

// automatically get state and setState props
const RGBChooser = enhance(({ state, setState }) => 
  <div>
    <input type="text" value={ state.red } onChange={ (e) => setState({ red: e.target.value }) } />
    <input type="text" value={ state.green } onChange={ (e) => setState({ green: e.target.value }) } />
    <input type="text" value={ state.blue } onChange={ (e) => setState({ blue: e.target.value }) } /> 
    <div
      style={{
        display: block,
        width: 30,
        height: 30,
        backgroundColor: `rgb(${state.red},${state.green},${state.blue})`
      }}
    /> 
  </div>
);

I have code, tests, and docs in this PR.

closes #321

@timkindberg timkindberg mentioned this pull request Apr 12, 2017
10 tasks
@istarkov
Copy link
Contributor

istarkov commented Apr 12, 2017

I'll write here why I would prefer something like #308 and not a current solution.
Because current solution causes new function creation for every handler. Yes we can always use withHandlers but see how it looks now

withState('something', 'setSomething', {a:1, b: 2}),
withHandlers({
  onAChange: ({ setSomething }) => ({ a }) => ({
      setSomething(prev => ({...prev, a}))
  })
})

As you see this is not so a big addition even you will write it inside render.
So I don't think solution above will somehow make better current situation.

I created an issue here reasonml/reason-react#37 about what I wrote in #308 BTW we can try implementation provided there https://github.com/reasonml/reason-react/blob/master/src/reactRe.re#L422-L454 and even it has some drawbacks now
I think it will be solved by React soon.

@timkindberg
Copy link
Contributor Author

I totally agree with you and are we sure it will be solved by React soon? Will they even settle on a final API soon as well? If not, why wait? Let's improve upon what we know now.

True, this is not a clever feature, but this is probably how withState should have behaved originally.

Typically I agree that most PRs should be rejected because they can be implemented in dev's own code, but this is simply allowing functional stateless components to have state features that mirrors existing class state features. For beginners of recompose it will be very comfortable, and it's way more flexible for when devs need to manage more than a single state variable.

@timkindberg timkindberg force-pushed the alt-state branch 2 times, most recently from 45812a1 to 24657af Compare April 14, 2017 02:36
@codecov-io
Copy link

codecov-io commented Apr 14, 2017

Codecov Report

Merging #357 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #357     +/-   ##
=========================================
+ Coverage   93.07%   93.27%   +0.2%     
=========================================
  Files          51       51             
  Lines         332      342     +10     
=========================================
+ Hits          309      319     +10     
  Misses         23       23
Impacted Files Coverage Δ
src/packages/recompose/withState.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25032fe...fab8c68. Read the comment docs.

@timkindberg timkindberg force-pushed the alt-state branch 2 times, most recently from 5aa5e3b to 31fd710 Compare April 14, 2017 02:46
@istarkov
Copy link
Contributor

but this is probably how withState should have behaved originally

agree, btw original React setState with state merging is also confusing, year or more ago I did
a PR to react-redux reduxjs/react-redux#50 so even superexperienced developer did a mistake with original setState

@istarkov
Copy link
Contributor

I wanna say current behavior is more predictable, and in a lot of cases allows to use it without withHandlers etc

@timkindberg
Copy link
Contributor Author

lot of cases allows to use it without withHandlers etc

When can you ever use it without withHandlers? Or do you mean use an anonymous function with the jsx?

So until we get additional setState version in React, all will be as is.

Can't we fix it ourselves instead of waiting on react? Can we throw and error to escape setState and have a wrapper/proxy setState that catches the error? In this way we could implement what you want with the updater approach.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

withState that has same API as React state
3 participants