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

Jump to bottom

Cannot use iterables in bindings #19945

Closed
sigmundch opened this issue Jul 10, 2014 · 7 comments
Closed

Cannot use iterables in bindings #19945

sigmundch opened this issue Jul 10, 2014 · 7 comments
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams.

Comments

@sigmundch
Copy link
Member

They could generate infinite loops. Right now we are OK with them because we are not discarding changes in 'get value' in polymer-expressions, but you'd hit the issue if we fix that. We also hit this if you use path_observers.

Here is a change that illustrates the problem:

https://codereview.chromium.org/382683005

ObservableMap.keys would always be different, so we would think there was a change every time we do a read. The CL patches ObservableMap.keys to cache it's previous result to illustrate that the issue goes away if we know somehow that the value didn't change.

I think we need to guarantee that with any Bindable, calling get value twice in a row should return either the same thing, or equal values.

Haven't we solved this issue before already?
@kasperl
Copy link

kasperl commented Jul 10, 2014

Added this to the 1.6 milestone.

@justinfagnani
Copy link
Contributor

Is this just for the dirty checking? If ObservableMap doesn't notify on #keys, it should be fine, no?

@sigmundch
Copy link
Member Author

It doesn't seem to be just for dirty checking.

I went back and continued refining that test. I can't repro with path_observers today (I must have misinterpreted a result yesterday), but I still see the problem with polymer_expressions.

Here is a simplified test case that illustrates the issue:

    <!DOCTYPE html>
    <html>
    <head>
      <script src="packages/web_components/platform.js"></script>
      <script src="packages/web_components/dart_support.js"></script>
      <link rel=import href="packages/polymer/polymer.html">
    </head>
    <body>
    <div id="result"></div>
    <input id="newval" type="text"/>
    <script type="application/dart">
      import 'dart:html';
      import 'package:observe/observe.dart';
      import 'package:polymer_expressions/polymer_expressions.dart';
      import 'package:template_binding/template_binding.dart';
    
      main() {
        var element = document.querySelector('#result');
        var input = document.querySelector('#newval');
    
        var tag = new Element.html(
            '<template>value: {{value}} [[bogus]]</template>');
        TemplateBindExtension.bootstrap(tag);
        var model = new Model(0);
        element.append(
            templateBind(tag).createInstance(model, new PolymerExpressions()));
    
        input.onChange.listen((e) {
          model.value = int.parse(input.value, onError: (_) => 0);
        });
      }
    
      class Model extends ChangeNotifier {
        Model(this._value);
    
        int get bogus => "";
    
        int _count = 0;
        int _value;
        int get value => _count++ >= 100 ? _value : _value++;
        set value(int value) {
          _count = 0;
          _value = notifyPropertyChange(#value, _value, value);
        }
      }
    </script>
    </body>
    </html>
    

The value displayed by "value: " is 100 initially, and x+100 when you enter some value x. If you remove the "bogus" binding, then we just use the value from the notification, but otherwise we evaluate the value of the expression.

Removed this from the 1.6 milestone.
Removed Pkg-Observe label.
Added Pkg-PolymerExpressions, PolymerMilestone-Next labels.

@sigmundch
Copy link
Member Author

Note: this can't be repro on its own, it requires the changes to polymer_expressions that you can see in the CL above.

@sethladd
Copy link
Contributor

Assigning owner as Priority is High. If this is done in error, please reassign or bump priority down. Thanks!

Set owner to @sigmundch.

@sigmundch
Copy link
Member Author

Removed the owner.
Removed Priority-High label.
Added Priority-Medium label.

@sigmundch sigmundch added Type-Defect area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. labels Jul 28, 2014
@DartBot DartBot mentioned this issue Jun 5, 2015
Closed
@DartBot
Copy link

DartBot commented Jun 5, 2015

This issue has been moved to dart-archive/polymer-dart#508.

@DartBot DartBot closed this as completed Jun 5, 2015
@kevmoo kevmoo removed the triaged label Mar 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@sethladd @kevmoo @justinfagnani @sigmundch @kasperl @DartBot