Skip to content

Fix concurrency issues in effectful form combinators #29

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

Merged
merged 1 commit into from
May 14, 2019

Conversation

arthurxavierx
Copy link
Contributor

asyncEffect and effect are used for effectful updates in specific parts of form data based on other parts of it. The old formulation of these combinators led to concurrency issues whenever two async updates would occur simultaneously based on a previous version of the form data (captured in a closure with withValue).

This pull request fixes this problem by enabling concurrency in asyncEffect and its derived combinators.

This is a breaking change.

@arthurxavierx arthurxavierx requested review from paf31 and maddie927 May 14, 2019 15:21
@arthurxavierx arthurxavierx self-assigned this May 14, 2019
@arthurxavierx
Copy link
Contributor Author

@paf31 I wonder if we should take this opportunity to remove the unused buildWithDefaults and replace build with buildConcurrently.

The problem with the latter is that build is used ubiquitously in our codebase at the moment.

Copy link
Member

@maddie927 maddie927 left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks

@maddie927
Copy link
Member

The old build function could be moved to the Lumi repo and given a build warning

@arthurxavierx
Copy link
Contributor Author

arthurxavierx commented May 14, 2019

Oh, that is an excellent idea @spicydonuts! Thank you!

@arthurxavierx arthurxavierx merged commit f794a5b into master May 14, 2019
@arthurxavierx arthurxavierx deleted the arthur/form/concurrent-effects branch May 14, 2019 18:27
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