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

[ng-core] [WrappedFormControlSuperclass] emitOutgoingValue should not be called implicitly #37

Closed
DmitryEfimenko opened this issue Apr 6, 2021 · 5 comments

Comments

@DmitryEfimenko
Copy link

I understand that the utility class attempts to reduce the boilerplate as much as possible and in doing so, it assumes that the sub class will want to call emitOutgoingValue any time when this.formControl.valueChanges emits a new value. However, this just may not be true for certain components.

Imagine creating a autocomplete component, which only allows values from the provided options. It should only emit a new value if the value typed into the control matches one of the options.

I think lines 56-58 need to be removed.

Unfortunately, that would be a breaking change.

@ersimont
Copy link
Member

ersimont commented Apr 7, 2021

@DmitryEfimenko If you happen to be available right now, I'm looking at this on my twitch stream. If you get this message very soon, please drop in and say hi so we can discuss! https://www.twitch.tv/ersimont

@ersimont
Copy link
Member

ersimont commented Apr 7, 2021

Too slow. It was right at the end of my stream. :)

If you don't mind, would you check out the video? Then you can respond to my thoughts here, or on Discord. You'll want to skip to around the 1:49:00 mark: https://www.twitch.tv/videos/978077129

@DmitryEfimenko
Copy link
Author

I did. It was great to watch your though process. It's a shame I missed the twitch invite :)

To answer your question "What is the control value when the user typed in an invalid value - something that does not match available options"?
The autocomplete component allows to configure this. This is configurable. All three scenarios are possible:

  • allow any typed in value to be emitted
  • emit null if the input does not match any options
  • do not emit new value (keep the previous valid value) if user input does not match any of the options

As you mentioned, the first and second scenarios are perfectly supported by the WrappedFormControlSuperclass. However, the third option - the one that is most commonly used in our application does not work.

I can also see your point when you were speculating whether the WrappedFormControlSuperclass is suitable at all for my use-case. But then you were saying that I might find useful the other 95% of the class - which is exactly the case! It would indeed be a shame if I could not use it.

I wonder if the functionality in question could be made optional and enabled by default. This way everybody would be happy and it would not be a breaking change.
On the flip side I love how small and clean the class is and I'd hate to bloat it. Although maybe it would not be too much.

If we were to add such customization, I think it would have to be in a form of a new method that would enable/disable this functionality.

Also, adding distinctUntilChanged() to the valueChanges totally makes sense to me. I don't think I've heard anything where Angular team decided to emit duplicates on purpose. I'd love to learn more about it.

Finally, I did join the Discord. Perhaps we could chat some time tomorrow.

@ersimont
Copy link
Member

ersimont commented Apr 8, 2021

What about overriding innerToOuter() to keep returning the latest valid value for as long as the current one is invalid? Would that do anything for you?

@DmitryEfimenko
Copy link
Author

definitely sounds like a possibility. But now we're getting into this situation of emitting duplicate values. I think the following change would make my use-case work:

const valuesToEmit$ = this.formControl.valueChanges.pipe(
  map(value => this.innerToOuter(value)),
  distinctUntilChanged()
);

this.subscribeTo(valuesToEmit$, (value) => {
  this.emitOutgoingValue(value);
});

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

No branches or pull requests

2 participants