feat(change_detection): treat List properties as pure

[chalin]

No description provided.

[chalin]

This PR recognizes List properties as pure. Thus, it is now possible to use, e.g., list.reversed without causing digest destabilization. By adding a single expression, this PR can also support Map properties, thus allowing map.keys and map.values to be used too (without indigestions).

[chalin]

FYI: Travis complains about 1/4 builds failing, but this failure comes from the master branch.

[chalin]

@mhevery, @vicb : btw, this is a "have your cake and eat it too" feature :) contributing greatly to offset #705. Treatment of purity need not be restricted to filters. Anything that is known to be pure should be processed in a uniform manner.

[vicb]

I don't like the name.
The class should not be aware of why/when it is called. isFirstPass means that collectChanges know it will be called multiple times.

[chalin]

I had a hard time thinking of a good name and settled on isFirstPass to start off with. How about resetDirtyArgs, or clearDirtyCache. Ah, I think that this is functionally accurate: clearMemoizationArgCache (cf. memoization).

[chalin]

Actually, clearMemoizationCache would do (as it is implicitly a caching of argument values).

[chalin]

Hmm, actually, we are just clearing a (dirty) bit, not the cached arg values. Maybe just clearDirtyArgs? Let me know which you prefer.

[vicb]

I like the idea but the naming seem wrong to me, see my inline comments.

[vicb]

"have your cake and eat it too"

Nice expression, I should try to remember it ;)

[chalin]

Addressed your concerns. Awaiting your feedback on which name you prefer. Here is another possibility: reMemoizeForPureFunc (memoization is done by default for pure functions, we clear the flag on the first pass).

[vicb]

Thanks. I need a little bit of thinking for the best name... I'll come back to you

[chalin]

Ok. In the meantime, I re-pushed edits and went with reMemoizeForPureFunc for now.

[mhevery]

this line will significantly impact the dirty checking speed. This is the core loop and it has to have minimal set of property accesses.

[mhevery]

There are two issues with this PR:

1. The implementation will slow down performance
2. I am not convinced this is the right thing.

We could fix 1 so I am not going to worry about this now.

I don't believe that it is the right thing because ever digest will cause a new reverse instance creation which will negatively affect the GC pressure. While it makes the stabilization assertion pass, it is not the right thing since it will allow a lot of garbage to be created on each digest.

Also there is no good way to enumerate all of the types which this will effect. For example same argument can be applied to Maps. This PR randomly special cases few types and makes them special which will make it hard to explain to people why are they special. Why does this work on List but not on my type which does the same thing.

People could subclass list and this behavior would be wrong.

There is a perfectly good solution to this, which is to use filters. For this reason I don't think this is an issue. I understand that this used to work, but the cost is too great and I don't think the cost outweighs the benefits. So I am going to close this PR without merging.

I know you worked hard on this PR, and I appreciate your work it just does not fit the vision we have for angular. I am sorry.

[chalin]

I appreciate you taking the time to respond. I thought that there might be a change that this would prove useful, but I understand that I don't have the overarching vision of the product evolution that you do.

Given that you favor filters, would you be open to the addition of general purpose filters to the AngularDart pre-declared filter set? E.g., right now there is lowercase (which essentially just wraps toLowerCase(), and uppercase; there was also the request for a keys filter and possibly a values filter. I think that the following general purpose filters might be useful:

• prop which could accept a String argument naming a property to be obtained from the filtered value. This could be used to access keys and values for Maps, reversed for Lists, etc.
• apply which could accept a function name optionally with other arguments. This could be used to perform the application of a (pure) function. It could replace lowercase, uppercase, and offer access to many more.

[chalin]

FYI, #771 is a concrete proposal of the ideas suggested in my last comment.