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

Some computed methods need non-contextual dependencies, it's not DRY to always list them #3299

Closed
sjmiles opened this issue Jan 15, 2016 · 17 comments
Assignees

Comments

@sjmiles
Copy link
Contributor

sjmiles commented Jan 15, 2016

Consider a translation scheme:

{{translate(translator, 'Hello World')}}

translate: function(translator, text, params) {
  return translator(text, params);
}

The design is that the 'translator` engine is injected by some means (simple property set, e.g.), therefore we must depend on 'translator' to trigger the relevant binding updates. Having to include this static name in every translate binding is a hassle.

One possibility is to add a way to register static dependencies for a computation.

{{translate('Hello World')}}

depends: {
  translate: 'translator'
},
translate: function(text, params) {
  return this.translator(text, params);
}

Something like that.

@robdodson
Copy link
Contributor

bonus points if this plays nice with behaviors

@TimvdLippe
Copy link
Contributor

I submitted an implementation in #3302 .

The implementation is a bit different that the proposal. I discussed this with @sjmiles and he agreed it was "a superior solution".

The example in the docs is:

<dom-module id="x-computed-with-dependency">
  <template>
    <span>{{methodName('literal')}}</span>
  </template>
  <script>
   Polymer({
     is: 'element-with-dependency',

     properties: {
       property1: {...},
       property2: {...}
     },

     dependencies: {
       methodName: ['property1', 'property2']
     },

     methodName: function(property1, property2, literal) {}
   });
 </script>
</dom-module>

I have not tested behavior support, but I think that could be implemented fairly easily. Could you elaborate on the specifications for behavior support? E.g. What should happen when behavior and child both declare the same or different dependencies?

@robdodson
Copy link
Contributor

Thanks for looking into this @TimvdLippe!

Could you elaborate on the specifications for behavior support? E.g. What should happen when behavior and child both declare the same or different dependencies?

For my use case I'd want the implementation for methodName and dependencies to both be part of the behavior that gets mixed in to the element.

As for parent and child declaring same or different dependencies, I haven't thought about that much... Maybe parent just overrides the required dependencies of child?

@sjmiles
Copy link
Contributor Author

sjmiles commented Jan 17, 2016

@samccone: unless I'm missing something, your pattern can't update the text in the <p> when the 'translate' method changes. This is a critical bit provide by the 'dependencies' solution.

@sjmiles
Copy link
Contributor Author

sjmiles commented Jan 17, 2016

@TimvdLippe: to follow current behavior rules, the dependencies objects should be (effectively) combined, everything else follows the standard inheritance model (i.e. any method implementations are 'last one wins', where 'last' means left-to-right in behavior arrays and the user's prototype at the end [the very last]). Conflicts in dependencies should also be handled as 'last one wins'.

@samccone
Copy link
Contributor

@sjmiles yep your right nm

@TimvdLippe
Copy link
Contributor

@sjmiles @robdodson Behavior support should now be added to the dependencies maps. Please check out this test

@robdodson
Copy link
Contributor

Cool so it looks like this covers this aspect:

Conflicts in dependencies should also be handled as 'last one wins'.

Are you also thinking of adding merged depenencies?

@TimvdLippe
Copy link
Contributor

@robdodson Merging of dependencies was already possible, but I forgot to add an explicit check for it. I have updated the test to show merging of dependencies works correctly.

@robdodson
Copy link
Contributor

cool! I'll defer to @sjmiles for approval

@sjmiles
Copy link
Contributor Author

sjmiles commented Jan 19, 2016

LGTM, will run it by @sorvell and/or @kevinpschaaf for extra care.

And ... nice work @TimvdLippe!

@kaste
Copy link
Contributor

kaste commented Jan 23, 2016

I'm a bit confused why this is called dependencies when what it does is: Iff this method is used as a view helper function, we inject these properties automatically for you. Or: when used as a view helper, you work with a partial/bound function. Within your normal JS there is nothing special about this method.

@TimvdLippe
Copy link
Contributor

@kaste The new behavior of this feature is that annotated computed functions can be updated whenever one of the global arguments is changed. In my eyes, inject means the notion of one-time injection of a dependency, whereas this feature guarantees continuous updates. Therefore I would still call it dependencies rather than injectees or alike. However, if the behavior could be better described we can of course change it.

@kaste
Copy link
Contributor

kaste commented Feb 1, 2016

Take #3358 as a rough concept. I really have no idea if it fulfills all and every requirement. On the other side it's in the same ad-hoc style as the rest of the Polymer source code.

Following example:

<dom-module id="x-bind-computed-property">
  <template>
    <div id="check">[[translate('Hello World.')]]</div>
  </template>
  <script>
    Polymer({
      is: 'x-bind-computed-property',
      properties: {
        translate: {
          type: Function,
          computed: '_computeTranslateFn(translator)'
        },
        translator: {
          type: Function,
          value: function() {
            return function(message) {
              return 'translated: ' + message;
            }
          }
        },
      },

      _computeTranslateFn: function(translator) {
        return function(message) {
          return translator(message);
        }
      },

    });
  </script>
</dom-module>

As you can see translate is a bound function in the sense that it is a property controlled by Polymer. In the example the property is a computed, but this is not required. You could use a complex observer that assigns a new function to translateas well.

As you can see 2: No further configuration is necessary. And although functions which return functions is always a bit meta and an advanced topic, I think any programmer can grab the concept without reading the documentation. I actually tried this before (I wanted to implement the filter function for a dom-repeat in this way), but it wasn't supported (which wasn't documented by the way). So this approach more likely fills a gap in Polymer.

@sorvell sorvell added p0 p1 and removed p0 labels Feb 4, 2016
@kevinpschaaf
Copy link
Member

We had a bunch of discussion on the Polymer team about the pros/cons of the alternate approaches by @TimvdLippe (#3302) and @kaste (#3358). The upshot is that people actually found it strange after thinking about it that observers/computing functions didn't have themselves as an implicit dependency, which is indeed an alternate way to solve the dependency currying problem for the translate use case as @kaste's PR showed. So I think I'll re-open #3358 and make a couple more suggestions there for improvements, and see if we can reach agreement on that approach.

kaste added a commit to kaste/polymer that referenced this issue Feb 12, 2016
kaste added a commit to kaste/polymer that referenced this issue Feb 12, 2016
@dfreedm
Copy link
Member

dfreedm commented Feb 17, 2016

Fixed with #3358

@dfreedm dfreedm closed this as completed Feb 17, 2016
@kevinpschaaf
Copy link
Member

For posterity, here's an example using the technique we merged (dynamic computing functions) to implement late-bound string translation via a behavior that results in a pretty pleasant end-user experience: http://jsbin.com/lajuci/edit?html,output

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

No branches or pull requests

8 participants