Skip to content
New issue

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

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

Already on GitHub? # to your account

Use form.getState() directly instead of using subscriptions. #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BowlingX
Copy link

Hi :),

I was running into timing issues on async error handling (with final-form 4.18.6 and react-final-form 6.3.3). (The promise in onSubmit would resolve before / at the same time the subscription callback fires and would compare against a stale state. I'm not sure why the subscription is needed because we have access to the form state directly with getState().

This pull request adresses the issue of a potential stale state.

@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #16   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines          44     40    -4     
  Branches       11     11           
=====================================
- Hits           44     40    -4
Impacted Files Coverage Δ
src/decorator.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 c26b46e...56dee29. Read the comment docs.

@BowlingX
Copy link
Author

Any more information required to have this merged? :)

@TylerRick
Copy link

I like how this simplifies things. Any reason we shouldn't merge this, @erikras?

@mishaberezin
Copy link

@erikras Could you please give some feedback on this issue. We're having the same problem in our projects and have to use fork of this library with similar patch applied.

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

Successfully merging this pull request may close these issues.

3 participants