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

Add property reflection for Polymer computed properties and observers #3184

Closed

Conversation

ChadKillingsworth
Copy link
Collaborator

Polymer observers and computed properties use string reflection. When property renaming is enabled, add property reflection calls to these strings such that they are renamed consistently.

Example:

class FooElement extends Polymer.Element {
  static get properties() {
    return {
      foo: {
        type: String,
        compute: '_computeFoo(bar)'
      },
      bar: {
        type: String,
        value: 'bar'
      }
    };
  }
  _computeFoo(bar) {
    return 'foo' + bar;
   }
}

The '_computeFoo(bar)' string will now be parsed and reflection calls added like:

JSCompiler_renameProperty('_computeFoo', /** @type {!FooElement} */ ({})) + '(' +
    JSCompiler_renameProperty('bar', /** @type {!FooElement} */ ({})) + ')'

@ChadKillingsworth
Copy link
Collaborator Author

cc @aomarks

@lauraharker
Copy link
Contributor

Internal issue http://b/122359057 created

@lauraharker lauraharker added the internal-issue-created An internal Google issue has been created to track this GitHub issue label Jan 4, 2019
@lauraharker lauraharker self-assigned this Jan 4, 2019
Copy link
Member

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

This is great!

@ChadKillingsworth
Copy link
Collaborator Author

I'm on vacation this week. I'll address the feedback next week.

@ChadKillingsworth
Copy link
Collaborator Author

@aomarks I've made the changes you requested

@lauraharker
Copy link
Contributor

Importing for internal review

@ChadKillingsworth
Copy link
Collaborator Author

@lauraharker @aomarks I have been unable to recreate the remaining issue. I've re-arranged the code a bit on a hunch and also updated the source references so that the warning should point to what exactly is triggering the warning.

If you can try this again with the latest changes, that will at least help me narrow down what's causing the type warning. It's a bit odd because I'm not changing any type signatures with this code.

@lauraharker
Copy link
Contributor

@ChadKillingsworth here's the new warning I get:

polymer/v2/polymer/lib/elements/array-selector.js:51: ERROR - variable ArraySelectorMixin.prototype.items redefined with type (Array|null), original definition at polymer/v2/polymer/lib/elements/array-selector.js:59 with type Array
  class ArraySelectorMixin extends elementBase {
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Here's array-selector.js immediately after the PolymerPass with --pretty_print and --preserve_type_annotations:
array-selector-after-polymer.txt

…pass to add them for us.

Prevents the compiler from assuming nullability on a property reference with no type annotation.
@ChadKillingsworth
Copy link
Collaborator Author

Thanks - that helps.

Here's my best guess: the compiler is treating the Element.prototype.propName; statements being used for a property sink as a nullability redefinition. That's surprising to me, but at least plausible.

I've refactored the code to add the synthetic sink statements directly rather than relying on the CheckSideEffects pass to add them. Hopefully this will solve the issue.

@lauraharker
Copy link
Contributor

That fixed it!

I'm running another set of internal tests now.

@lauraharker
Copy link
Contributor

Or no, that was a false positive.

I now get a different error:

{SyntheticVarsDeclar}: ERROR - Variable JSCOMPILER_PRESERVE declared more than once. First occurrence: {SyntheticVarsDeclar}

@lauraharker
Copy link
Contributor

The latest commit fixes the error I was seeing. 👍
I'm doing another round of global testing now.

@lauraharker
Copy link
Contributor

Submitting this change internally

@blickly blickly closed this in 4cd6295 Feb 6, 2019
@ChadKillingsworth ChadKillingsworth deleted the polymer-improvements branch February 18, 2019 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes internal-issue-created An internal Google issue has been created to track this GitHub issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants