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

Support property observers which are direct function references #4574

Merged
merged 1 commit into from
Dec 1, 2017

Conversation

ChadKillingsworth
Copy link
Contributor

Partially addresses #4543

Provides better static analysis and refactoring support in multiple tools. Alleviates the need for property reflection with Closure-compiler renaming.

The test case included isn't actually that useful (but does test the change). Real benefits come with the class style syntax:

class FooElement extends Polymer.Element {
  static get is() { return 'foo-element'; }
  static get properties() {
    return {
      bar: {
        type: String,
        observer: FooElement.prototype.barChanged_
      }
    }
  }
  barChanged_(newValue, oldValue) {
    console.log(newValue, oldValue)
  }
}

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

This SFTM. Could you resolve the merge conflicts and then @kevinpschaaf can probably take a look?

…dition to strings.

Provides better static analysis and refactoring support in multiple tools. Alleviates the need for property reflection with Closure-compiler renaming.
@ChadKillingsworth
Copy link
Contributor Author

Sorry for the delay. Rebased to tip of tree.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Talked about this PR with @kevinpschaaf In general we feel this is a small quality of life improvement by using the language semantics. We were initially reluctant, as this PR introduce a new way for the same thing. However, as the benefits are substantial and for this case it is small, we are okay with the change.

We need to update the docs for this change as well @arthurevans

@aomarks
Copy link
Member

aomarks commented Dec 25, 2017

Filed Polymer/old-docs-site#2421 to add documentation.

@aomarks
Copy link
Member

aomarks commented Dec 25, 2017

Filed Polymer/old-docs-site#2421 to add documentation.

Oops, ignore me, missed that there already was an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants