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

Makes sure that the last captured input is used to run validations instead of the first input #63

Merged
merged 2 commits into from
Jan 27, 2020

Conversation

skress
Copy link
Contributor

@skress skress commented Jan 23, 2020

This change fixes the bug that is described as 'problem 1' in #48 (comment).

…n old snapshot.

unsafeRunValidationVariant in Internal.Transform now returns a function that modifies the form such that this transformation can be run against the current value of the form instead of the old one which was captured before the (possibly async) validations have been run.
@skress
Copy link
Contributor Author

skress commented Jan 24, 2020

I've updated the PR, it should now also fix the second problem.

@thomashoneyman
Copy link
Owner

@skress Thank you so much for your work on this -- as you can tell, it's not a trivial problem and I haven't been sure how to fix it so far. I won't be able to fully review your work until tomorrow, over the weekend, but just wanted to thank you for putting in the effort and let you know that it's on my radar.

@thomashoneyman thomashoneyman self-assigned this Jan 24, 2020
Copy link
Owner

@thomashoneyman thomashoneyman left a comment

Choose a reason for hiding this comment

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

Great work tracking this down.

I can see how this addresses the case where no input is entered, validation and atomic complete, and the last value caught by atomic gets written instead of the actual last value. And it also addresses the case where an old state is updated instead of the most current state when validation finishes. Thanks for the clear code!

@naglalakk I am going to go ahead and merge this as I believe it addresses your issue. However, if you have a chance to test this out yourself I'd appreciate hearing from you about whether your issue is resolved.

Thanks again to both of you for opening and solving the issue!

@thomashoneyman thomashoneyman merged commit b42d54e into thomashoneyman:master Jan 27, 2020
@skress skress deleted the bugfix branch January 27, 2020 10:01
@naglalakk
Copy link

Hi @thomashoneyman, @skress. I just verified this and this PR is working 100% for me. Thank you so much for following up on this and @skress for tackling this. Great work! <3

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