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

Remove EmitChangesDirective #60

Closed
hotforfeature opened this issue Sep 29, 2017 · 6 comments
Closed

Remove EmitChangesDirective #60

hotforfeature opened this issue Sep 29, 2017 · 6 comments
Labels
Milestone

Comments

@hotforfeature
Copy link
Owner

I'm not currently happy with the state of the EmitChangesDirective. It's unintuitive and does not offer enough value for the complexity it brings.

Current Implementation

The one and only purpose of this directive is to enable banana-in-a-box binding. It is 100% possible to do two-way data binding without this directive.

Without emitChanges

@Component({
  selector: 'app-polymer',
  template: `
    <paper-checkbox [checked]="checked" 
        (checked-changed)="checked = $event.detail.value"></paper-checkbox>
  `
})
export class PolymerComponent {
  checked: boolean;
}

With emitChanges

@Component({
  selector: 'app-polymer',
  template: `
    <paper-checkbox [(checked)]="checked" emitChanges></paper-checkbox>
  `
})
export class PolymerComponent {
  @PolymerChanges() checked: boolean;
}

Motivation

I want to either remove or completely re-work this directive for v2. There are a few problems with how [emitChanges] currently works.

Performance

Once you add [emitChanges] to an element, it will add event listeners for every notify: true property for that element. This is never the real world use case, we only want to listen to the property we're using two-way data binding for.

I considered doing something like <paper-checkbox emitChanges="checked"> to specify which properties to listen for, but at that point we're losing a lot of the value for [emitChanges]: simplicity with [( )] binding.

Complexity

I often forget to add the @PolymerChanges() decorator to a property that uses two-way binding. You don't need it for [(ngModel)] since the IronControlDirective handles it, and I avoid adding it on one-way bound properties since they don't get changed by Polymer.

To use [( )] syntax you must

  1. Add [emitChanges] to the element
  2. Add @PolymerChanges() to the Angular property
  3. Optionally implement onPolymerChange to get notified of changes

Value vs [(ngModel)]

We have an extremely large and complex Angular/Polymer app. In our massive app we are using the value provided by EmitChangesDirective exactly once. And even looking at this one case, we could replace it with [(ngModel)].

Most two-way data binding situations I've encountered have been a use case where [(ngModel)] could also fill the role, such as checkboxes.

<paper-checkbox [(checked)]="checked"></paper-checkbox>
<paper-checkbox [(ngModel)]="checked"></paper-checkbox>

Proposal

Every time I sit down and think about how to make this directive better, I come back to wanting to get rid of it. Banana binding syntax is moving away from #usetheplatform. It's complex, and it will often silently fail if the developer doesn't realize a property they're trying to bind to doesn't emit change events.

After many months working in Angular and Polymer, I find little value to this directive. I'd like to hear some feedback on where others find value using it as opposed to manually wiring up two-way data bindings or using [(ngModel)].

@hotforfeature hotforfeature added this to the 2.0.0 milestone Sep 29, 2017
@BorntraegerMarc
Copy link
Contributor

There is no way to only listen to changes emitted for only checked when defining <paper-checkbox [(checked)]="checked" emitChanges></paper-checkbox> instead for all notify: true?

@hotforfeature
Copy link
Owner Author

Not that I can tell. I failed to find a way to determine what input/output properties were bound at runtime.

@BorntraegerMarc
Copy link
Contributor

Then I totally agree with the measurement. Especially for the performance aspect...

@alexeygt
Copy link

alexeygt commented Oct 2, 2017

Fully support the change.

@alexeygt
Copy link

alexeygt commented Oct 2, 2017

Just searched through our entire app - we never use banana syntax for specific properties, only with [(ngModel)]. I think an example with (checked-changed)="checked = $event.detail.value" is enough.

Plus it removes pain with @ _PolymerChanges - although I've used it only during testing of origami few month ago.

@hotforfeature hotforfeature changed the title Refactor or Remove EmitChangesDirective Remove EmitChangesDirective Oct 13, 2017
@hotforfeature
Copy link
Owner Author

Going to go with removing this in v2 and adding documentation to help developers with two-way binding

hotforfeature added a commit that referenced this issue Oct 16, 2017
BREAKING CHANGE: emitChanges has been deprecated for poor performance. Either use form controls (ngModel) or manual two-way binding.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants