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

Use this.props.state as previousState for first action response #28

Closed
wants to merge 1 commit into from
Closed

Conversation

danny-andrews
Copy link

Fixes #27.

@danny-andrews
Copy link
Author

Don't accept this yet. I found the root of the problem. this.props.previousState starts out as null.

@danny-andrews
Copy link
Author

There we go. :)

@danny-andrews danny-andrews changed the title Only call select when state is truthy Use this.props.state as previousState for first action response Jan 16, 2016
@@ -31,6 +31,7 @@ export default class LogMonitorEntry extends Component {
constructor(props) {
super(props);
this.handleActionClick = this.handleActionClick.bind(this);
this.previousState = this.props.previousState || this.props.state;
Copy link
Owner

Choose a reason for hiding this comment

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

This is incorrect. Component can receive new props in componentWillReceiveProps so it is wrong to calculate anything based on props in the constructor and cache it.

Copy link
Owner

Choose a reason for hiding this comment

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

In general, this seems to give you the wrong semantics. You are making it look like the previous state is the current state for the first action. However what really happens is that it doesn’t exist.

gaearon added a commit that referenced this pull request Feb 3, 2016
@gaearon
Copy link
Owner

gaearon commented Feb 3, 2016

Thank you again for the PR and taking a look at this.
I applied what I believe to be a more correct fix in 3af3102.

@gaearon gaearon closed this Feb 3, 2016
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.

2 participants