ngrx / store

RxJS powered state management for Angular applications, inspired by Redux
MIT License
3.9k stars 311 forks source link

Unable to establish true one-way binding to view #228

Closed sky-coding closed 7 years ago

sky-coding commented 8 years ago

We've been building an enterprise application using ngrx/store and have run into some issues that could be deal-breaking, so I've spent today trying to work out some of the problems; the main one right now is that user input mutates the view even when it doesn't affect the model.

For example, some checkbox's value (checked/unchecked) is determined by a calculation. When the user clicks the checkbox, it cannot simply two-way bind a value back. So we use [ngModel], capture the change using (ngModelChange), and allow the reducer to take action and then recalculate the value of the checkbox. The problem is that if the checkbox's resulting model remains unchanged, the user's action will still check/uncheck the box even if the model says that the user's action should've had no effect.

I've created a simplified example of the problem using inputs:

http://plnkr.co/edit/KIHLhXfnCOybN5EWFxMA?p=preview

In this demo, (only) Misko Hevery's description is not editable due to business logic that the component is unaware of.

If you try the "No trackBy" section, you will see that user input for his description does not affect the model, and also does not affect the input... but this is because Angular is redrawing the DOM for this entire section, which allows it to correctly reevaluate the expressions, but this is not usable.

When trying the other two sections, editing Misko's description does not affect the model, but the input's value still reflects whatever the user has typed. It will continue to do so until the model changes.

I've seen this behave properly in React Redux, and I'm wondering how we can use one-way binding in Angular 2 if we cannot stop the user from mutating their own view. All advice/blogs/examples are welcome, thanks!

P.S. I noticed the readme demo link is using out of date versions, here is a bare-bones plunker I created today as a starting point, it's running the latest of Angular 2, ngrx/store, and ngrx/store-devtools: http://plnkr.co/edit/D7VZb3?p=preview

kylecordes commented 8 years ago

It seems to me that this challenge are hitting comes because of a touch point between the Ngrx/Redux/Elm/CQRS-in-the-UI/etc way of looking at the world, and the API and behavior presented by the DOM. As far as the letter is concerned, the user action to request the change of state of a checkbox, immediately result in that change, without any help from a more global perspective on state taken by a developer.

At such a boundary, I think we might just have to accept a bit of a hack. I have not tried this, but here is the first thing I would try. Coincidentally with handling the click of the checkbox which conveys the action into the state system, also set a timeout (probably a zero duration timeout) and then just forcibly set the UI state of the checkbox back to whatever the state system says it should be.

This is not a great answer, and I hope it gets superseded by a great answer. But I have done something quite similar to this before, it took only a couple lines of code, and it resulted in a very local solution to a local API/behavior challenge. It avoided contaminating or even changing the rest of the application state mechanism to work around this bit.

Would something like that work for your situation?

Also more broadly, I wonder if Angular itself (probably not Ngrx?) could be enhanced to establish a kind of persistent one-way binding, some way of requesting that Angular check and reapply the binding at each "turn" instead of trusting that the previously bound settings will stay as they were. Or if that is too much scope for Angular itself, I wonder if we could write a directive which could be stacked on in the somewhat unusual cases where this comes up, that achieves that same thing generically.

laurelnaiad commented 8 years ago

See also https://github.com/angular/angular/issues/4062

EDIT: wait, maybe I'm confused. Did you want one-way or two-way binding? The issue title says one-way, but it seems like you're aiming for two-way?

This statement confuses me:

but this is because Angular is redrawing the DOM for this entire section, which allows it to correctly reevaluate the expressions, but this is not usable.

What do you mean 'not usable'?

sky-coding commented 8 years ago

@kylecordes Thanks for your thoughts, looks like you understand my issue. What we have in place now is a little less hacky than using a timeout, and I'll explain.

So, the core mechanic I am running into is this:

@Component({
  template: `<input [ngModel]="foo" /> `
})
export class AppComponent {
    foo:string = 'bar';

    ngOnInit() {
      setInterval(() => {
        this.foo = 'bar';
      }, 1000)
    }
}

In this demo, the input's value will never be forced to show "bar" again once the user changes it. The input will always reflect the user's keypresses, and not the actual model it's bound to. For me, this means that we have to use two-way binding with [(ngModel)], and allow the user's value to bind to something before it can be reverted. Since I am not binding to state directly, I can just add two-way binding to my model:

<input type="text" [(ngModel)]="record.customerDescription" (change)="updateDescription(record, $event.target.value)" />

updateDescription(record:DashboardRecord, description:string) {
  this.store.dispatch({type:UPDATE_CUSTOMER_DESCRIPTION, payload:{id: record.customerId, description:description}});
}

And now, on blur, the input's value will be reset to reflect the model, due to record.customerDescription being changed. The actual model in state is not affected.

This works okay, but my 2 concerns are the use of 2-way binding, and state/computed fields not recalculating until the user changes fields.

@laurelnaiad, see my component example above, it seems impossible to avoid two-way binding, but I would like the inputs to reflect the state of the model one-way, regardless of user actions.

Also, what I mean by not usable, is that it is a side effect of binding to immutable arrays, and that re-rendering hundreds of DOM elements on keypress is too expensive to be usable. Also, because the elements are re-drawn, the user loses focus on the input they are typing into, as you can see in the demo.

My understanding is that React does not have this same low-level problem, and I'm questioning the viability of the redux pattern in Angular 2 if we cannot avoid the use of two-way binding at the component level.

laurelnaiad commented 8 years ago

The angular iterator looks for iterables (be they immutable or not) and "does the right thing" with them. It is inherent in the strictly immutable nature of redux that if anything about one of the elements has changed in the iterable, that a new reference would be there for that member, and that otherwise the references would be identical, at which point angular should not attempt to modify existing members of the dom that are based on that element of the iterable within the template that's doing the iterating.

I'm not so sure there's an actual problem here, if I'm understanding correctly... but you do have to make sure that you're sending the signals you really intend, and not just with timing of the observables and when they push at the dom, but also with the references to each sub-element in your state tree lining up. I've found I can control the timing that change detection runs right down to levels at which I actually don't even care, and conversely I haven't found cases where I couldn't get the mix I needed if I wrote my selectors right.

laurelnaiad commented 8 years ago

If you're worried about changes that are pushed at DOM elements propagating right back through to actions, just make sure to .distinctUntilChanged() your elements' values or otherwise check that they're actually different in a filter, so that if a push lands on them with identical content, it doesn't do any actual triggering b/c the value is the same.

kylecordes commented 8 years ago

@sky-coding Okay, I think I understand your description of the situation a bit more now. What behavior are you hoping for, from the overall piece of software that does this? In other words, when a user edits a field, if your application does not push that edit all the way through such that the [] binding would simply set the same value that is already there anyway, how should it behave? In other words, under what conditions would you expect that a user would type some text in an input field, but then have that text change be rejected and changed back her the original [] binding?

I don't think the situation is as different for you here in Angular versus react as one might first guess. There may be some relatively minor low-level difference in how react handles changes to items that are typically treated as directly editable by the underlying browser (such as input text fields and checkboxes), but I suspect we can get any desired behavior with either one easily enough.

One thing that comes to mind, have you tried using the two different halves of NgNodel separately? Something like this?

<input type="text" [ngModel]="record.customerDescription" (ngModelChange)="updateDescription(record, $event.target.value)" />

I think this is closer to what you seem to be aiming for, than using the separate change event. I don't know if makes any actual difference - but at least you are then using NgModel in both directions.

I have an idea for the simpler checkbox case that we were discussing earlier in this thread, if I get time I will tinker with that idea and plunk her and see if it yields anything. It so happens that I have hit a feature need an application close to your original checkbox question, so now my curiosity is pique as to what can be done most easily and effectively.

@laurelnaiad I think the essence of the question wasn't so much about avoiding pushing a change of more than once, but rather how to have a change pushed again transparently and automatically, when the DOM element being pushed to also has a built-in behavior that changes the value behind the scenes when the user does something. In other words, how are we supposed to use one-way bindings appropriately, when binding to properties that can and do change according to forces other than the binding we attach?

laurelnaiad commented 8 years ago

Gotcha. Makes sense.

the DOM element being pushed to also has a built-in behavior that changes the value behind the scenes when the user does something.

So in ngrx, the latter would ultimately be taking place via an action.

The former (the pushing of an update down into the down) would happen via a selector.

I think what you're saying is that you don't want the action which is attached to your DOM element to (necessarily) result from cases where the element gets a new value pushed.

What I'm trying to suggest is that if you are observing the element, and nothing has changed, then a well-observed element won't get very far. Your dumb component would raise some sort of event out to a smart component, perhaps, where the smart component would be subscribed to a stream of such events and seeing that nothing has changed and thereby dead-ending the thing you're worrying about before you get there.

Does that make any sense?

Like

theseValuesFromTheEvents$
  .distinctUntilChanged()
  .subscribe(evt => this.store.dispatch(createAction(evt)))

or something like that. If you generally follow a pattern of not dispatching events when there's nothing to do, you can head it off there.

Alternatively, you can figure out that it's a no-op in an effect or a reducer (using the same technique of 'oh, this hasn't changed'). Either way, You do indeed want to make sure you don't create an infinite loop.

Me? I don't try to do anything when textbox values change unless it's behind a throttle so that no needless effort is exerted trying to do almost nothing, and otherwise I don't worry about it because I can do a generalized filter in place at the smart component level that's kinda like the above to make sure I'm not chatty with altogether spurious actions.

kylecordes commented 8 years ago

@laurelnaiad Related to your last comment at the end, about throttling and so on, here's where I have landed on forms for all of our own stuff, and for our curriculum:

Treat NgModel is a legacy-ish feature, think of the template forms system as something to ease the transition for developers coming from Angular 1 or another two-way binding, local-component-state framework. For new code, don't use it, use reactive forms instead. I have no idea whether this will become a popular way of working in Angular 2 though. It seems that most tutorials for A2 introduce NgModel pretty early.

However I don't think template versus reactive forms really addresses the topic here, which is that we have these DOM elements which behave in a two-way binding sort of way regardless of what architecture we pile on top of them, to change the user makes ends up going directly into the properties of the elements immediately without being pushed there, and without really any direct control from, any higher-level dataflow mechanism (like NgRx) we put on top of it. So I'm going to experiment with this a bit later when I have time tried to get a better handle on it, like I wrote it is relevant to things I work on and I am thrilled to see it brought up here for discussion, thank you @sky-coding .

laurelnaiad commented 8 years ago

Ah, well I have the luxury have not having to write a curriculum for a brand new platform, just yet, so I've not had to make any policy rulings on how to go about things. I'm still letting this particular issue fester organically with a variety of techniques in our app while the remainder of the known 'make it reactive' bits are worked out in angular forms.

kylecordes commented 8 years ago

@sky-coding Take a look at this plunker. It is for about the simplest possible case - a checkbox. The user can click on the checkbox as usual, but its state is determines by the state management in a Store, and it not directly toggled by the user's click. Two bits needed:

http://plnkr.co/edit/xzCyse?p=preview

Does this do the desired thing for the checkbox case? What is the comparable thing for a text field?

I wish I had a good answer in hand for "do this binding after the browser is done with this click" other than setTimeout. Maybe someone else has a better answer.

sky-coding commented 8 years ago

Thank you both for your input. @laurelnaiad, I am not having an "actual problem" with binding, eventing, etc.. I have been able to use ngrx/store while implementing everything that I need. However outside of very simple use cases, the implementations are not clean (e.g. setTimeout, return false). It's also not been very consistent, it's hard to ask multiple teams to use ngrx/store without providing a generic pattern that can be used across all components. Having to add trackBy to all *ngFors (due to immutability) alone has been such a hassle.

I think the essence of the question wasn't so much about avoiding pushing a change of more than once, but rather how to have a change pushed again transparently and automatically, when the DOM element being pushed to also has a built-in behavior that changes the value behind the scenes when the user does something. In other words, how are we supposed to use one-way bindings appropriately, when binding to properties that can and do change according to forces other than the binding we attach?

You nailed it here, I'm just trying to better understand DOM-level binding and address a potential technical limitation of one-way binding, which was most simply demonstrated in https://github.com/ngrx/store/issues/228#issuecomment-249450837 using setInterval(() => { this.foo = 'bar'; }, 1000)

@kylecordes your plunker scenario perfectly demonstrates a use case, thank you. Your solution is very similar to ours, and it gets the job done, but to me it feels clunky and does not scale very well.

You mentioned reactive forms, and I feel like .valueChanges.subscribe and .setValue might be a much cleaner way to handle this use case, and any other generic "change/react" scenario. I've been wanting to give reactive forms a try, but I've been hung up on the fact that Angular internally mutates the form instances, meaning that you cannot have them in store. And at that point, the components are not using one-way binding, the application state is outside of store, and ngrx/store could be removed without anybody noticing. Maybe that's my answer.

laurelnaiad commented 8 years ago

Maybe I missed the point. If you are using setTimeouts and assigning to instance properties in them, then it doesn't really sound like a reactive scenario.

I had assumed you were trying to avoid the things you say you're doing in favor of doing one way reactive rendering with reactive reactions to Dom events instead of ngModel bindings to instance properties. If you really wanted ngModel reading from instance properties, then I think I was on the wrong page, so I apologize for the distraction.

kylecordes commented 8 years ago

@laurelnaiad There is more than one timeout/interval under discussion. If I understand right, this one:

setInterval(() => { this.foo = 'bar'; }, 1000)

...is there to explain the notion of repeatedly "reminding" Angular that a certain property has a certain value, and illustrating that this property does not get pushed again via a binding to an element, if that element had its property change behind the scenes. In other words, an Angular binding only works for properties that are not prone to change by themselves behind the scenes.

The other timeout/interval:

  toggle() {
    setTimeout(_ =>
      this.store.dispatch({ type: TRY_TOGGLE }));
    return false;
  }

...illustrates another side of that "change behind the scenes" ideas. Here, the notion is that, by dispatching the action outside of the event handler, it becomes possible for Angular to then bind back to a property of the DOM element which caused the event. Without the timeout in this latter snippet, Angular attempts to bind a value into the element, but the DOM ignores that attempt because the element is still in the process of emitting the event. So there are two aspect of this "hole" in binding:

What I would really like is this this latter timeout was not necessary. I'm not sure if that could be achieved without breaking some other important Angular behaviors, which is why I'm chatting about it over here in the ngrx repo rather than raising an issue on Angular itself. (It was also pointed out by @sky-coding that React avoids this problem.)

Doing this makes it possible to take a more fully reactive approach, that approach works in the plunkr I posted above but with this irritating hack of setTimeout in the event handler. I wish there was a way to do this that was more automatic, if I get sufficiently curious I will go dig around what React does - a wild guess is that its synthetic event system already handles these events "after" the underlying browser event rather than in the midst of it as we get by default with Angular. That could be completely wrong of course, I have not looked.

MikeRyanDev commented 7 years ago

Seems like this has been resolved. Please reopen and leave a comment if it is not.