Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Ease Angular adoption: support a *simple* CDA based on equality (in addition to the new CDA) #705

Closed
chalin opened this issue Mar 11, 2014 · 4 comments

Comments

@chalin
Copy link
Contributor

chalin commented Mar 11, 2014

Executive summary

While I used to be able to write a single ng-repeat collection expression like names.split(',') in my HTML file, it seems that under Angular's new Change Detection Algorithm (CDA) I have to replace that by a custom 10-line-or-so @NgController that implements caching. This will not aid AngularDart adoption. I admired Angular because it allowed me to focus on web app design rather than writing boilerplate code, but this seems to be a step backwards. Details are given below.

A solution I propose is to have Angular support at least two CDAs: one which is the simplest possible to use (automatic family sedan version) and then the new CDA (turbocharged formula 1 version, ... or think "Jeep without doors or safety belts") targeted for experts ;).


DETAILS

I believe that a selling point of Dart is that it gives its users choices. Consider types as one example: to quote Gilad Bracha in his concluding remarks to Optional Types in Dart:

If you hate types, you need not use them at all. ... If you love types, you may use them everywhere

Since January I have been teaching a graduate course in Dart and AngularDart. Up until the the introduction of the new Change Detection Algorithm (CDA) of v0.9.9, I was able to develop considerable lecture material supported by examples without any surprises. In particular, it was pleasant to see that simple web apps were simple to write.

I understand that the new CDA is highly optimized so that it can support:

  • 20,000 field checks and
  • 300,000 array element checks

per millisecond [1], but this comes at the cost of rendering many simple intuitively-correct web apps either completely invalid or requiring considerable rewrite. I give an example of each case below.

Example: Simple app that is now invalid

I have a suite of elementary examples illustrating basic functionality of AngularDart without the use of custom controllers. Here is one example:

Team member names (comma separated):<input type="text" ng-model="names">
<ul ng-if="names != null">
  <li ng-repeat="name in names.split(',')">{{name}}</li>
</ul>

This runs nicely under 0.9.8:

image

Now under the new CDA we get the following error:

Observer reaction functions should not change model. 
These watch changes were detected: names.split(","): [Alice,  Bob] <= [Alice,  Bob]

The message is complaining that the list [Alice, Bob] isn't the same as the list [Alice, Bob]. This will confuse most newcomers. What is happening is that the new CDA doesn't like that the expression names.split(',') yields a new list instance every time it is evaluated --- despite the fact that it generates the same list value every time.

Under the new CDA, I see no solution to this other than writing a customer controller. Let's try that next.

Example: Restrictions on custom controllers

A first try at writing a custom controller to solve the problem illustrated above might be:

@NgController(selector: '[team-name-controller]', publishAs: 'ctrl')
class TeamNameController {
  String names;
  List<String> get nameList => names == null ? [] : names.split(',');
}

with corresponding changes to the HTML

Team member names (comma separated):<br>
<input type="text" ng-model="ctrl.names">
<ul ng-if="ctrl.names != null">
  <li ng-repeat="name in ctrl.nameList">{{name}}</li>
</ul>

But this yields the same warning we got before: Observer reaction functions should not change model.

I raised this issue earlier (#641), in which I gave a different example of a controller that generated a list of int values:

List get list => new List.generate(3, (i) => i)

and the response was that under the new CDA I need write something like

class MyController {
  List list;

  MyController(MyService service) {
    service.onDataChanged.listen((_) {
      _updateData();
    });
  }

  _updateData() {
    list = new List.generate(3, (i) => i);
  }
}

Is this really the simplest possible solution under the new CDA? While I admired Angular because it allowed one to focus on web app design rather than writing boilerplate code, this seems to be a step backwards. This will not help the adoption of AngularDart.

A Possible Solution

The main problem, IMHO, is the belief that "one size fits all" when it comes to CDA. I think that there should be at least two CDAs:

  • one CDA which is the simplest possible to use and is based on element equality, including special treatment for collections;
  • the new CDA for diehard web app writers needing to support the watch and update of tens of thousands of elements

with the former being the default.

[1] Change Detection. Status: Draft. Accessed March 11, 2014. Author: Misko Hevery

@caitp
Copy link
Contributor

caitp commented Mar 11, 2014

This is an issue in AngularJS with the 1.x scope implementation as well (although ng-repeat uses a special watch algorithm to make this less of an issue), but functions returning new object instances causes model instability, and that's relatively well understood.

It can be very expensive to perform an exact comparison between the old object instance and the new object instance, so it's sort of hard to get this right.

/subscribes to thread, interested in thoughts on this

@mhevery
Copy link
Contributor

mhevery commented Mar 12, 2014

So the issue with the names.split(',') is that it will produce a lot of GC pressure. It is fine for demo apps but as soon as one builds large scale apps it becomes a problem, especially on mobile. The issue is that people do not realize that names.split(',') is expensive and then when the app is slow they blame angular.

But in your case there is a simple solution. Just define a split filter and then write names | split:','. This will work since filters are consider pure functions and they will only recompute if the inputs to them changes.

@chalin
Copy link
Contributor Author

chalin commented Mar 13, 2014

Btw, I understand the arguments regarding efficiency and I am in agreement with them.

Let me state this issue another: in the Change Detection document you point out that AngularDart's design should (or could?) support different CDAs (possibly plugged-in via DI or provided natively be a VM).

Currently, as far as I can tell, the design does not support CDA plug-in because some core parts of the CDA are duplicated in the code; e.g., _EvalWatchRecord and DirtyCheckingRecord check() methods both contain code like:

    var last = currentValue;
    if (!identical(last, current)) {
      if (last is String && current is String &&
          last == current) {
        // This is false change in strings we need to recover, and pretend it
        // is the same. We save the value so that next time identity will pass
        currentValue = current;
      } else if (last is num && last.isNaN && current is num && current.isNaN) {
        // we need this for the compiled JavaScript since in JS NaN !== NaN.
      } else {
        previousValue = last;
        currentValue = current;
        return true;
      }
    }
    return false;
  }

Thanks for mentioning function purity, that gives me a few ideas (and it helps me that this bug is now fixed).

@chalin
Copy link
Contributor Author

chalin commented Mar 20, 2014

Not consistent with the current Angular design goals.

@chalin chalin closed this as completed Mar 20, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

3 participants