Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add resetState, reset to powerplug #148

Closed
wants to merge 10 commits into from
Closed

Conversation

istarkov
Copy link
Contributor

Ref #110

see comment why

I've added in first commit eslint flow plugin as otherwise it's hard to edit flow file in editors like vscode (flow file is one big eslint error)

cc @TrySound please check and i'll proceed with other Components

.eslintrc Outdated
"extends": [
"eslint:recommended",
"plugin:import/recommended",
"plugin:react/recommended"
"plugin:react/recommended",
"plugin:flowtype/recommended"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this config care too much about formatting. It's easier to add this rules

    'flowtype/define-flow-type': 'error',
    'flowtype/use-flow-type': 'error',

@@ -21,21 +21,25 @@ import { State } from 'react-powerplug'

## State Props

**initial = {}** _(optional)_
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't trim spaces please, they are used to add line breaks

Relax flow rules
@istarkov istarkov force-pushed the master branch 2 times, most recently from 052c268 to 0c436b4 Compare August 10, 2018 09:50
renderProps(props, {
value: state.value,
set: value => setState(s => ({ value: set(value, s.value) })),
reset: () => resetState(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not reset: resetState?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By analogy, State has setState with callback, so resetState has too, but all other components has no callback, so here we remove callback too

@istarkov
Copy link
Contributor Author

istarkov commented Aug 10, 2018

The possible issues with current reset implementation is that if object Shape will be changed in initial,
setState inside reset will merge new Shape with previous instead reset.

To avoid we need to make instead of this

class State extends Component {
  state = {
    ...this.props.initial,
  }

this

class State extends Component {
  state = {
    value: { ...this.props.initial },
  }

It's fun but it was the first PR I made to react-redux ;-) moving state into value

@istarkov
Copy link
Contributor Author

istarkov commented Aug 10, 2018

Fixed in above setState implementation

@istarkov
Copy link
Contributor Author

istarkov commented Aug 10, 2018

BTW for my own case, I don't need that reset for every helper, and if we add Prop types to setState at State helper updater, I can reset State without resetState method, just using

setState((_, {initial}) => initial)

So feel free to close the PR, as it so-so needed ;-) and for my case (having that I have stable object shapes) result can be achieved just making few flow type changes

@istarkov
Copy link
Contributor Author

Close in favor of #151

PS:
TODO:
Pick flow eslint from this PR and create new

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

Successfully merging this pull request may close these issues.

None yet

2 participants