feat(computedFrom): allow usage on method

[bigopon]

This PR

• follows Jeremy's comment
• enhances computedFrom decorator with error message when used on normal field
• changes 1 test in call-scope spec, where it test binding shouldn't observeProperty.
• addresses: parametrical computed / recursive observation (make methods observable) parametrical computed / recursive observation (make methods observable) #147

Usage similar to computedFrom on getter:

class MyClass {
  @computedFrom('firstName', 'lastName')
  showWelcomeMessage(country) {
    return `${messages[country].hello} to ${this.firstName} ${this.lastName}`;
  }
}

• Demo

@EisenbergEffect @jdanyow @davismj @jsobell

[davismj]

I apologize, I failed to close out the issue I had opened up long ago.

It looks good. However, I'm not sure it solves any problems that couldn't already have been solved. See here for an updated demo:

https://gist.run/?id=0aa5bd84d78994e11e435eba811ab556

[bigopon]

Putting parameters on the view to observe changes will be enough most of the time. Ability to add observers from view model will be a nice addition. It may help make the view clearer. For our examples, firstName & lastName are internal dependencies so putting them into the view makes less sense than having it declared in the view model. I think having them both will be a good thing.

[davismj]

As far as I can tell, since both @computedFrom and the function parameter strategy both have the same variables available to it, @comptutedFrom methods is just another way of doing exactly the same thing.

I guess my biggest concern is that it is a mix of the two strategies. If you have a method, you're likely passing an argument in already. One developer might write some sketchy code that another developer is forced to maintain, where a method takes a couple variables in, and computes from a couple variables as well. Perhaps that will introduce confusion to the second developer, wondering what the difference is between the two.

As I said, they seem to be solving exactly the same problems with exactly the same tools, so I can't say it's a bad idea. It's only flaw is that its a bit late to the party. I probably won't use it, and I probably will continue to advise others to use the function parameter strategy. I don't think there is any reason not to include it, though.

[jdanyow]

I don't think we should add the overhead to connect for such an edge case. There are so many better ways of doing this sort of thing.

for example, this:

class MyClass {
  @computedFrom('firstName', 'lastName')
  showWelcomeMessage(country) {
    return `${messages[country].hello} to ${this.firstName} ${this.lastName}`;
  }
}

Is this in the view:

<span>${messages[country].hello} to ${firstName} ${lastName}</span>

[jsobell]

The reason this remains a requirement is because by insisting that every dependency of a function be included in the view, we're polluting the view with implementation dependencies.
There are a few other examples (and workarounds) here: http://www.sobell.net/aurelia-dirty-checking-a-function/
This is definitely not something that should be brushed off, and it's been requested regularly in the chat groups for the last two years.

[davismj]

@jsobell @bigopon @jdanyow

There are a few other examples (and workarounds) here: http://www.sobell.net/aurelia-dirty-checking-a-function/

I love what you've done in this article. But the one thing that is missing is my recommended strategy of passing dependencies to the function. For your "Uh-oh", your code would have become one of the following, value converter being my preference for this case:

<div>delimit(fullName, ',')</div>
<div>fullName | delimit: ','</div> <!-- best practice -->

delimit(text, delimiter) {
  text.split(/\s/).join(delimiter)
}

The reason this remains a requirement is because by insisting that every dependency of a function be included in the view, we're polluting the view with implementation dependencies.

I think this is the best argument. But even so, I see this really awkward push to take everything out of your view. I know your case is pedantic, but even so, I think the above examples are much more readable and usable than the alternative that would come out of this addition:

<div>delimitFullName(')</div>

@computedFrom('firstName', 'lastName')
delimitFullName(delimiter) {
  return [this.firstName, this.lastName].join(delimiter);
}

it's been requested regularly in the chat groups for the last two years.

It's a bit of a balancing act. If we took every request that I've seen, Aurelia would not be as clean and usable as it is today. Of course if we ignore every request, we won't stay relevant. As I mentioned above, I don't think this is horrible or even bad, and it wouldn't be a huge mistake to include it. I just think it's unnecessary and sub-optimal. I see this as more of a training issue than a gap in the existing tools.

[bigopon]

I was doing this PR with code reuse-ability in mind. By having a way to describe only the varying dependencies (parameters) in the view without losing observe-ability , I can easily call it the same method from the view model, thus avoid littering my view model with methods that do almost the same thing. Plus, in complex views in my app, often times I had to extract expressions into methods to make the view more readable. Of course my design could have been cleaner, but it could have been certainly handy.

I think @jdanyow 's comment about performance is the strongest argument against this.

[jsobell]

@davismj I'm aware of why we prioritise or ignore change requests, and there are workarounds for problems like the one listed here. The case I'm describing is a real-world situation that crops up repeatedly, and moving logic into the view is not usually a well thought out option.

In fact, indexes or any form of iterator are the worst culprit in this type of issue.
Here's a chunk of code from a permission handler in one of our management utilities:

                  <div repeat.for="p of perms" class="col-xs-6" if.bind="allowed($index)">
                    <div class="media clickable" click.trigger="togglePerm($index)">
                      <div class="media-left">
                        <button if.bind="enabledPerm($index)"
                          class="pill ${['pill--red','pill--green','pill--light-red','pill--light-green'][displayPerm($index, permissions, mask)]}">
                          ${['Deny','Allow','(Inherit)','(Inherit)'][displayPerm($index, permissions, mask)]}
                        </button>
                        <button if.bind="!enabledPerm($index, permissions, mask)"
                          class="disabled pill ${['pill--red','pill--green','pill--light-red','pill--light-green'][displayPerm($index, permissions, mask)]}"
                        >
                          ${['Deny','Allow','(Inherit)','(Inherit)'][displayPerm($index, permissions, mask)]}
                        </button>
                      </div>
                    </div>
                  </div>

Examples of the functions bound to are:

  displayPerm(i: number, ...triggers) {
    return this.getBitSeq(this.perms[i].bits);
  }

  enabledPerm(i: number, ...triggers) {
    return (this.perms[i].bits & this.enabledpermissions)!==0;
  }

  allowed(i: number) {
    return (this.visiblepermissions & Math.pow(2,i))!==0;
  }

In this case, ...triggers is needed and values of permissions, mask have to be passed back from the view to force the updates in the UI. How does a UI designer know that those 'magic' variables names are required, and what if a new variable is referenced in one of those base-class methods? Hence why the pedantic example isn't quite as pedantic as it first appears :)

Whether we like it or not, this is a situation that people hit, and we can work around it as shown, we can tell them they have to implement triggers manually, or we can implement dirty-checking.
Of those, dirty-checking is the only transparent solution that doesn't require modifying the view simply to overcome our lack of ability to update a binding automatically when a dependant variable is changed. We have it for properties, and if properties supported parameters we wouldn't be discussing this.

If finding a solution creates performance issues on all bindings then we obviously shouldn't do it that way, but if a solution can be found that only impacts on decorated function, I don't see any issue.
If not, and we're really not going to implement a solution to this situation, we to clearly document the workarounds.

[atsu85]

@jdanyow

${messages[country].hello} to ${firstName} ${lastName}

How would you handle i18n with this approach?

[davismj]

@jsobell It's hard for me to answer your post. Perhaps we can reach out and talk about it. My gut feeling is that there might be a better approach in a few ways for what you're doing above. For example, this code:

displayPerm(i: number, ...triggers) {
   return this.getBitSeq(this.perms[i].bits);
}

Might better be written:

displayPerm(i: number, permissions: any[], mask: number) {
  let perm = permissions[i]; // note that I'm clearly using the dependency on `permissions`
  return this.getBitSeq(perm.bits);
  // and i have a feeling this should be return this.getBitSeq(perm.bits, mask); 
  // thus invoking the dependency on mask, making these variables no longer "magic"
}

Notice, I haven't made any changes to the logic or the variables available to either the view or the view-model. I'm just simply did the exact same thing while clearly writing down (a) there is a dependency on permissions and (b) I'm actually using that dependency.

I've said multiple times that I don't think @computedFrom is necessarily bad, it's just a different and potentially sub-optimal approach. If we used it in this particular case, however, I think it would in fact lead to much less straightforward code:

${['Deny','Allow','(Inherit)','(Inherit)'][displayPerm($index)]}

@computedFrom('perms', 'mask')
displayPerm(i: number) {
  return this.getBitSeq(this.perms[i].bits);
}

In this case there are a few problems. First of all, when looking at the view I have no idea that there are dependencies other than $index. Second, when looking at the JavaScript I now have an idea how i and perms are being used. I still have no idea where, how, why, or if mask is being used. If mask wasn't being used, I have no idea why and I'd be afraid to touch it. That's why I would say this is sub-optimal.

By the way, I think we solved the very same problem you're trying to solve here in this question. When we were done, permissions bindings looked like this:

<div auth="disabled.bind: canEdit" disabled.bind="!editing">

It was a tricky problem and needed a tricky solution, but I think what we ended up with is extremely easy to develop with.

[jsobell]

At the nuts and bolts of the situation, we have a system that doesn't have a means of updating binds to
functions automatically, but can to property accessors (in which case they are polled or @computedFrom), or standard object fields.
For functions we've decided all dependencies must go via the view or the user has to intercept writes to external dependencies and raise a trigger, and that's a bit messy. Not only is there no reason the view should know anything about the underlying model, and forcing passing on 5 variables simply because we don't have a way of detecting changes otherwise is a bit messy.

The code link you posted is an interesting way of binding, but is still based on a single field, isDisabled or authorized. Try and include that code in a loop where $index is required to be passed and it appears we end up back in the same situation:

<div auth="disabled.bind: canEditFn($index)" disabled.bind="!editingFn($index)">

or

<div auth="disabled.bind: canEdit[$index]" disabled.bind="!editing[$index]">

(which opens up a whole new can of worms :) )

Since there are so many situation where this is unsuitable or unnecessarily complex, why don't we use the same polling option on functions that we provide by default on properties? After all, if it's really a once-only call to the function it will be decorated with one-time, in the same way that property getters have to be marked up.
Sure, we can point people to append a custom & Dirty() or somesuch to do this, but it would be much cleaner if it was a standard feature given that it's a standard requirement.