Use scope.watchAST instead of scope.watch #1127
Conversation
| ..watch('[a, 2, 3]', (value, previous) => logger(value)) | ||
| ..watch('{a:a, b:2}', (value, previous) => logger(value)) | ||
| // TODO what do we do with this case? | ||
| // ..watchAST(parser(''), (value, previous) => logger(value)) |
There was a problem hiding this comment.
@jbdeboer What should we do when watching an empty string (''). Parser can't deal with this one yet. Should we modify the parser a bit or not allow watching this specific value?
There was a problem hiding this comment.
in AngularJS, we can denote parsed expressions as constant, which instructs them to be unwatched after they're first evaluated.
I'm not sure if there's a similar mechanic in angular.dart, but it seems reasonable that it should behave similarly. (note: currently, AngularJS won't treat a parsed empty string as a constant expression, and the returned function is basically a noop, returns undefined --- so maybe null in dart?)
|
@markovuksanovic I would leave the tests as is: scope.watch() is basically scope.watchAST(parser(xx)), so we aren't gaining much from adding the explicit call in the tests. Plus if makes with PR much less scary. |
mvuksano
changed the title
|
@jbdeboer I've reverted changes to tests. This is now ready for review. |
|
The ASTs should be precomputed so we can avoid creating them in the critical path. |
| @@ -219,7 +220,7 @@ class ElementBinder { | |||
| if (directive is AttachAware) { | |||
| var taskId = tasks.registerTask(); | |||
| Watch watch; | |||
| watch = scope.watch('1', // Cheat a bit. | |||
| watch = scope.watchAST(_astParser('1'), // Cheat a bit. | |||
|
@jbdeboer I tried computing the |
| this.componentData, this.decorators, | ||
| this.onEvents, this.bindAttrs, this.childMode); | ||
| this.onEvents, this.bindAttrs, this.childMode) { | ||
| _helperAst = _astParser('1'); |
There was a problem hiding this comment.
Compute this in a global service, ElementBinderBuilder, instead of re-computing it for each ElementBinder.
|
Thanks @markovuksanovic; the '1' change is ElementBinder is good (after fixing the comments). Could you break that into a separate SHA? For the directives, what we really want is to deprecate the use of Scope.watch() entirely by e.g. adding APIs to the ElementBinder's attribute mappings. That is a much more involved change though. We have been thinking about it a bit: e.g. in NgClass, we are using Scope.watch() instead of attribute mappings because we need to set collection and canChangeModel on Scope.watch, that is not possible with attribute mappings right now. Instead of adding a cache, we should fix the API. |
This is done in the bind- branch, right ? |
Fixes #1110