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

Clarify that computed properties and observers should only rely on their dependent properties, not `this`. #1156

Closed
arthurevans opened this Issue Jun 8, 2015 · 9 comments

Comments

Projects
None yet
3 participants
@arthurevans
Contributor

arthurevans commented Jun 8, 2015

From @m4b on June 8, 2015 4:22

Issue

I've noticed that the order of declaration for polymer properties seems to matter in an arbitrary manner when a computed property uses another property (like an array) using this in the function definition, instead of passed as a parameter.

This is perhaps bad practice (I don't see it explicitly proscribed anywhere in the docs (yet)), etc., but it's a nasty gotcha; it might be a good idea to just throw an error, if possible, whenever a property is used not via a parameter.

For example:

    <dom-module id="init-good">
        <template>
          <style>
          </style>
        </template>
        <script>
          Polymer({
            is: "init-good",
            properties: {
              _title: {
                type: String,
                computed: '_getTitle(_selected)'
              },
              _array: {
                type: Array,
                value: function(){
                  return ["one", "two", "three"];
                }
              },
              _selected: {
                type: Number,
                value: 0,
              }
            },
            _getTitle: function(selected){
              return this._array[selected];
            },
            ready: function(){
              console.log("ready: ", this._array, this._selected, this._title);
            }
          });
        </script>
    </dom-module>

<init-good> runs fine. However, if the declaration positions of _array and _selected are switched, then we get "Uncaught TypeError: Cannot read property '0' of undefinedPolymer._getTitle", i.e.:

            properties: {
              _title: {
                type: String,
                computed: '_getTitle(_selected)'
              },
              _selected: {
                type: Number,
                value: 0,
              }
             },
              _array: {
                type: Array,
                value: function(){
                  return ["one", "two", "three"];
                }
              }

The "good" order of _array and _selected, compared to the error-producing order seems arbitrary, no matter which way you look at it (whether the properties are initialized from top to bottom, or bottom to top).

Clearly _title needs _selected and _array, but neither _selected or _array needs anything.

Hence if initialization occurs from the top, then switching _selected with _array should have no effect, and in both cases it should throw an error. But if initialization occurs from the bottom, then switching _selected and _array should both not throw an error, since _getTitle hasn't been initialized yet.

Due to the arbitrariness, if the user accidentally writes or amends a computed definition using this instead of passing another property as a parameter, it will sometimes work, and sometimes not work. Hence the suggestion to throw an error always. If it's bad practice, and has strange declaration order problems, then it should probably be discouraged as much as possible by refusing to run at all.

Steps to Reproduce

I can reproduce this on Linux (firefox and chrome), and OSX (on firefox, chrome and safari).

  1. Put the above <init-good> dom module into a project, import it, and create the element.
  2. Note no error occurs.
  3. Switch the declaration order of _array and _selected, and note the undefined error coming from _getTitle
  4. Various permutations show that most orders throw an error, except when _array is at the "top" (the first declared property) of the 3 property declarations.

Copied from original issue: Polymer/polymer#1776

@arthurevans

This comment has been minimized.

Contributor

arthurevans commented Jun 8, 2015

From @kevinpschaaf on June 8, 2015 16:30

Computed properties should declare all their dependencies to avoid being called with undefined. Note that _getTitle references both _selected and _array, so it should declare both as dependencies, i.e.

            properties: {
              ...
              _title: {
                type: String,
                computed: '_getTitle(_array, _selected)'
              },
              ...
            },

            ...

            _getTitle: function(array, selected){
              return array[selected];
            },

This behavior of not calling computed functions until all dependences are defined is discussed here:

The computing function is not invoked until once all dependent properties are defined (!== undefined). So each dependent properties should have a default value defined in properties (or otherwise be initialized to a non-undefined value) to ensure the property is computed.

I will keep this issue open and assigned to our docs lead @arthurevans to make sure the nuance about needing to declare all dependent properties is clear.

Also (pro-tip), in order for _getTitle to be re-computed when the array is mutated (e.g. splice, etc.), observe _array.* using a deep path observer:

            properties: {
              ...
              _title: {
                type: String,
                computed: '_getTitle(_array.*, _selected)'
              },
              ...
            },

            ...

            _getTitle: function(change, selected){
              return change.base[selected];
            },

@arthurevans arthurevans self-assigned this Jun 8, 2015

@arthurevans arthurevans added the docs label Jun 8, 2015

@arthurevans

This comment has been minimized.

Contributor

arthurevans commented Jun 8, 2015

From @m4b on June 8, 2015 16:59

@kevinpschaaf thanks for the response. I'm aware the dependencies need to be declared, I was more worried about the declaration order arbitrarily throwing errors if a property is referenced with this. Yes, it shouldn't be done, but it's a nasty shock for the reasons I outlined above. Adding a note in the documentation should be a fair warning, at the very least.

So, while not a computed property per-say, a similar issue occurs with an observer (which I don't believe declare their dependencies?).

Correct me if I'm wrong, but observer functions are allowed to perform arbitrary work in the function definition using this, correct?

For example, the following module runs fine, but if the declaration order of _array and _selected are switched, it will throw errors.

<dom-module id="init-observer">
  <template>
    <style>
    </style>
    <paper-button on-tap="_change">tap me</paper-button>
    <iron-pages selected="[[_selected]]">
      <template is="dom-repeat" items="[[_array]]">
        <h1>[[item]]</h1>
      </template>
    </iron-pages>
  </template>
  <script>
    Polymer({
      is: "init-observer",
      properties: {
        _array: {
          type: Array,
          value: function() {
            return ["one", "two", "three"];
          }
        },
        _selected: {
          type: Number,
          value: 0,
          observer: '_selectedChanged'
        },
        _title: {
          type: String,
          computed: '_getTitle(_selected, _array)'
        },
      },
      _selectedChanged: function(selected){
        console.log("_selectedChanged selected: ", selected);
        console.log("_selectedChanged array: ", this._array[selected]);
      },
      _change: function(e) {
        this._selected = (this._selected + 1) % 3;
      },
      _getTitle: function(selected, array) {
        return array[selected];
      },
      ready: function() {
        console.log("ready: ", this._array, this._selected, this._title);
      }
    });
  </script>
</dom-module>

What I take away from this is that declaration order is important, at least for observers, and a note should be made somewhere to avoid surprises like the above.

@arthurevans arthurevans changed the title from property initialization/declaration order arbitrarily throws errors in computed value definitions using `this` to Clarify that computed properties and observers should only rely on their dependent properties, not `this`. Nov 6, 2015

@volodymyrrudyi

This comment has been minimized.

volodymyrrudyi commented Jan 9, 2016

Hey guys, it would be great to add this notice, since recently I've seen this issue in several popular components, like Google Maps (e.g. GoogleWebComponents/google-map#242)

@dnadales

This comment has been minimized.

dnadales commented Jan 14, 2016

I think this also applies for property observers. If these observers depend on other properties, then the order of declaration of these property matters. Do the docs prescribe a way-of-working regarding this?

@volodymyrrudyi

This comment has been minimized.

volodymyrrudyi commented Jan 14, 2016

@capitanbatata in case of property observers, they will be only executed once all properties enumerated in the observer declaration are set. But a similar issue may happen in such observers when you are referencing something, that is not included in the list of observed properties.

@dnadales

This comment has been minimized.

dnadales commented Jan 14, 2016

Oh! That is good to know @volodymyrrudyi! Did I missed that in the documentation?

@volodymyrrudyi

This comment has been minimized.

volodymyrrudyi commented Jan 14, 2016

It's mentioned in the docs for 1.0 under the Observing changes to multiple properties section:

To observe changes to a set of properties, use the observers array.
These observers differ from single-property observers in a few ways:
Observers are not invoked until all dependent properties are defined (!== undefined).

@dnadales

This comment has been minimized.

dnadales commented Jan 14, 2016

I did miss it. Thanks.

@arthurevans

This comment has been minimized.

Contributor

arthurevans commented Apr 30, 2016

Dup of #1492, fixed in #1522.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment