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

Reduce calls to onChange of the parent form #215

Merged
merged 2 commits into from
May 5, 2017

Conversation

amannn
Copy link
Contributor

@amannn amannn commented May 5, 2017

I've noticed that when FormsyText is used in a controlled form where there's an onChange listener registered on the form, the listener is called three times currently.

I was able to reduce the calls to two so far, but I'm not sure where the other call is coming from.

I'm currently using a workaround like this:

onFormChange = (formData, didValueChange) => {
  if (!didValueChange) return;
  ...
}

But I'd say it's much cleaner if the callback is only called when there was actually a change.

Copy link
Collaborator

@ryanblakeley ryanblakeley left a comment

Choose a reason for hiding this comment

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

This seems like a good idea.

@amannn
Copy link
Contributor Author

amannn commented May 5, 2017

Thanks for approving! But the test currently fails, as onChange is called twice. I'm not sure where the second call is coming from. Maybe there's something in formsy-react that triggers this and we can't fix it in formsy-material-ui. Do you have an idea?

If we can't find out, I could change the test to expect(calls.length).toBeLessThan(3); so the test works and we've at least got one call less.

@ryanblakeley
Copy link
Collaborator

  1. Knocking off 1 of those calls is a marginal improvement. So the test you proposed works for me.
  2. It very well could be that formsy is triggering the extra call. It looks like the place to test that hypothesis is probably in this Mixin.

@amannn
Copy link
Contributor Author

amannn commented May 5, 2017

I've updated the test assertion. If I find some more time I'll try to find the root cause in the mixin you linked. Thanks!

@ryanblakeley ryanblakeley merged commit f2601b3 into formsy:master May 5, 2017
@amannn amannn changed the title [WIP] Reduce calls to onChange of the parent form. Reduce calls to onChange of the parent form. Sep 21, 2019
@amannn amannn changed the title Reduce calls to onChange of the parent form. Reduce calls to onChange of the parent form Sep 21, 2019
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.

2 participants