felangel / bloc

A predictable state management library that helps implement the BLoC design pattern
https://bloclibrary.dev
MIT License
11.8k stars 3.39k forks source link

The form example has problems #1888

Closed enyo closed 3 years ago

enyo commented 3 years ago

Apart from the fact, that it doesn't build with the latest flutter version anymore, the flutter_form_validation example is problematic.

To debounce the change events of the input fields, this is the transformEvents function used:


  @override
  Stream<Transition<MyFormEvent, MyFormState>> transformEvents(
    Stream<MyFormEvent> events,
    TransitionFunction<MyFormEvent, MyFormState> transitionFn,
  ) {
    final debounced = events
        .where((event) => event is! FormSubmitted)
        .debounceTime(const Duration(milliseconds: 300));
    return events
        .where((event) => event is FormSubmitted)
        .mergeWith([debounced]).switchMap(transitionFn);
  }

The problem with that is, that the switchMap discards all previous events when a new event occurs. With 300ms that might not be that big of a problem, but it can still occur.

So two scenarios are problematic here:

  1. The user changes the value of one input field, then quickly switches to the next and enters data there (which can happen easily when using Tab). This would discard all change events from the first input field.
  2. The user makes changes to an input field and hits enter (or clicks the submit button). This would discard any changes made to the input field.
enyo commented 3 years ago

Even if the problem with the switchMap is removed, there remains an issue, where hitting the submit button would not prevent the debounced input event from arriving, but it could happen, that the form is submitted before the debounced event arrives (which could be disastrous obviously).

It would be great if somebody could provide an example that behaves correctly, since this example will result in many people implementing dangerous forms.

felangel commented 3 years ago

Hi @enyo πŸ‘‹ Thanks for opening an issue!

Regarding the first issue, what flutter version are you using and what error are you seeing? Regarding the second issue, thanks for bringing that up, I'll address it ASAP πŸ‘

enyo commented 3 years ago

I'm using the pre version 1.23.0 and for some reason the named arguments of the widgets aren't recognised:

image

But I'm realising that this is probably some preversion/non-nullable issue that should be easily fixable on my part.

I would be interested in how you solve this problem with the streams. Do you have an idea on how to fix this easily and elegantly?

felangel commented 3 years ago

@enyo I just updated the form validation example to incorporate inline validation best practices and improve the user experience. Please let me know what you think, thanks! πŸ™

felangel commented 3 years ago

Closing for now since I feel the original issue was addressed. Feel free to comment with any questions or feedback and I’m happy to continue the conversation πŸ‘

enyo commented 3 years ago

Just looked at it, and I think it's good now. Would have loved to see an example with debouncing because I'm curious on how this could have been solved cleanly with rxdart, but that's another topic :)

felangel commented 3 years ago

@enyo awesome, yeah it's tricky because the debounce inherently leads to the issues you described.

enyo commented 3 years ago

@felangel Thanks for your swift help and responses in the last few days. I have now started to implement this form validation pattern in my app and it works well, but it is a lot of boilerplate code for each field. I have just written 3 forms, each of them having 3-6 input fields, and the blocs just become quite big with very repetitive code that is prone to simple mistakes.

These two functions have to be repeated for each field in a form:


  Stream<AuthenticationState> _emailChanged(EmailChanged event) async* {
    final email = Email.dirty(event.email);
    yield state.copyWith(
      email: email.valid ? email : Email.pure(event.email),
      status: Formz.validate([email, state.password]),
    );
  }

  Stream<AuthenticationState> _emailUnfocused(EmailUnfocused event) async* {
    final email = Email.dirty(state.email.value);
    yield state.copyWith(
      email: email,
      status: Formz.validate([email, state.password]),
    );
  }

When adding a field, each of these functions needs to be updated so the Formz.validate call includes all values, and making a mistake with .dirty() or .pure() somehwere would also break the behaviour.

I have thought about ways to fix this issue but wasn't able to come up with a satisfying solution. I started writing a generic valueChanged() function on the *State class, that then would iterate over all props and see which field needs to be updated and then have the changed/unfocused logic in one simple place but this has become quite complex and obscure for such a simple functionality so I'm hesitant to increase the complexity like this.

Long story short: do you have any ideas on how to improve this code, so it is less repetitive and error prone?