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

fix: call setState before callbacks #690

Merged
merged 2 commits into from Oct 13, 2018

Conversation

leogiertz
Copy link
Contributor

What kind of change does this PR introduce?

  • bugfix
  • feature
  • refactoring / style
  • build / chore
  • documentation

Did you add tests for your changes?

  • Yes, my code is well tested
  • Not relevant

If relevant, did you update the documentation?

  • Yes, I've updated the documentation
  • Not relevant

Summary

A potential fix for: #689

Does this PR introduce a breaking change?

Nope

Other information

@codecov
Copy link

codecov bot commented Oct 11, 2018

Codecov Report

Merging #690 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #690   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files           3        3           
  Lines         226      226           
  Branches       68       68           
=======================================
  Hits          224      224           
  Misses          2        2
Impacted Files Coverage Δ
src/index.js 99.45% <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 f37fb5b...9d59799. Read the comment docs.

src/index.js Outdated
// Update `acceptedFiles` and `rejectedFiles` state
// This will make children render functions receive the appropriate
// values
this.setState({ acceptedFiles, rejectedFiles })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, this still does not have any guarantee that the cb fn will be called after the state is set (.setState() is async).

What you want is to have the onDrop* cb fns called in the .setState({...}, () => /* call the onDrop* fns here */) cb fn 😄

Also, add a test for this.

src/index.js Show resolved Hide resolved
Copy link
Collaborator

@rolandjitsu rolandjitsu left a comment

Choose a reason for hiding this comment

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

Also, rebase your branch as there are some conflicts that prevent us from merging.

src/index.spec.js Outdated Show resolved Hide resolved
@leogiertz
Copy link
Contributor Author

Ugh, wrong button.

@rolandjitsu rolandjitsu merged commit b6aab86 into react-dropzone:master Oct 13, 2018
@okonet
Copy link
Collaborator

okonet commented Oct 13, 2018

🎉 This PR is included in version 6.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

3 participants