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

Connect refactor to use transactional setState() #373

Closed
wants to merge 8 commits into from

Conversation

slorber
Copy link
Contributor

@slorber slorber commented May 1, 2016

This is a pull request for #368

Not to be merged yet, as the code is not clean but it's enough to get some feedback, as all the existing tests are passing

this.setState({

// TODO before setTimeout was not required. can I modify this test safely? it seems not a bid deal
setTimeout(() => this.setState({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only modified test @gaearon but I think it's ok to modify

@@ -47,7 +37,6 @@ export default function connect(mapStateToProps, mapDispatchToProps, mergeProps,

const finalMergeProps = mergeProps || defaultMergeProps
const { pure = true, withRef = false } = options
const checkMergedEquals = pure && finalMergeProps !== defaultMergeProps
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tgriesser can you take a look at that plz? I've been able to make the tests pass without using this variable. Is there a missing test-case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the premise was that if we use defaultMergeProps, we know that it’s not going to be shallowly equal if either stateProps or dispatchProps changed. Therefore not even worth doing a shallowEqual check.

@@ -208,13 +173,23 @@ export default function connect(mapStateToProps, mapDispatchToProps, mergeProps,
}
}

componentWillMount() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaearon I've been able to remove the store subscription here, but I still need to be able to trigger the computation from here, otherwise nothing can be put into state for the first render and we are back to doing the computation in render()

is that ok?

@slorber
Copy link
Contributor Author

slorber commented May 4, 2016

@gaearon did you have time to check my PR?

I've tried to run benchmarks of @mweststrate . Not sure to have done them correctly, but I actually see increased performances :) It's not huge, and maybe I did something wrong during the benchmark.

# Change 1:
- Before: ~50ms
- After: ~45ms

# Add 1:
- Before: ~110ms
- After: ~103ms

Maybe you can run them yourself and confirm?

I've published the benchmark app before and after so that anyone can take a look

@gaearon
Copy link
Contributor

gaearon commented May 4, 2016

Hey, great work. I’m super busy this couple of weeks so I’ll leave this hanging for now. If all tests are passing and you’re confident about it, we can cut a beta.

@slorber
Copy link
Contributor Author

slorber commented May 5, 2016

thanks :)

I'm going on holiday 2 weeks until the 21 so you will have some time to check the code :)

I'm confident globally but not really about the checkMergedEquals attribute that @tgriesser added. Don't really understand the usecase as all my tests are passing without using that, but it has probably be done for a good reason so...

@timdorr
Copy link
Member

timdorr commented Aug 14, 2016

Superseded by #416

@timdorr timdorr closed this Aug 14, 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.

3 participants