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

polymer properties -- need Property<T> to support synchronous notifications #18343

Closed
jmesserly opened this issue Apr 21, 2014 · 16 comments
Closed
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams.

Comments

@jmesserly
Copy link

as implemented by observe-js Observer.createBindablePrototypeAccessor and Observer.bindToInstance.

googlearchive/observe-js@762f600

I've been ignoring this problem for a while because final Property&lt;T&gt; fieldName = new Property&lt;T&gt;(val); is more verbose than @published T fieldName = val;, and it adds verbosity to usage. But I don't think we can anymore. On the bright side, we can more easily experiment with other ways of doing notifications.

@jmesserly
Copy link
Author

Also, maybe this is a better pattern to encourage:

final _fieldName = new Property<T>(val);
T get fieldName => _fieldName.value;
T set fieldName(x) { _fieldName.value = x; }

... it's even worse at declaration time, but avoids the ".value" for access.

@blois @­sigmund -- thoughts here on best looking API?


cc @blois.
cc @sigmundch.

@jmesserly
Copy link
Author

correction to above:

  • T set fieldName(x) { _fieldName.value = x; }
  • set fieldName(T x) { _fieldName.value = x; }

@blois
Copy link
Contributor

blois commented Apr 21, 2014

Ideas on how this would impact memory usage? In a system with an extremely rich set of bindable properties but which only a fraction of which are actually set, there's going to be quite a bit of overhead of these per-instance Property fields.

If there's a base-class for these then there could be sparse storage of non-default properties in a dictionary on the base, using something such as:
static const _fieldName = const Property<bool>(#fieldName, initialValue);
bool get fieldName => _fieldName.get(this);
void set fieldName(bool value) { _fieldName.set(this, value); }

Alternatively, may be able to do:
bool get fieldName => get(#fieldName, orElse() => defaultValue);
void set fieldName(bool value) { set(#fieldName, value); }

@jmesserly
Copy link
Author

Removed Area-Polymer label.
Added Pkg-Polymer, Area-Pkg labels.

@sigmundch
Copy link
Member

I'm trying to think of other alternatives, but I'm not sure I have the whole picture yet. Do we need both sync and async notifications, depending on how the property is used? Is this needed for Observable, or only for Bindable?

Some ideas I'm trying to think about is whether this could this be addressed by adding a second API that provides the other notification timing. For example, '.changesSync' in Observable, or 'openSync' in Bindable?

@jmesserly
Copy link
Author

we need a property that redirects the getter/setter to another object, immediately. And is configured at runtime.
https://github.com/Polymer/observe-js/blob/master/src/observe.js#L1277 that one and bindToInstance right below it.

The only way I could think of to get that indirection is having an object for indirection.

We could do a transformation, but... it would have to be on in development mode too :(. I'm not really a fan of that, personally... At that point, it feels like we're inventing language syntax instead of just optimizing for deployment. But if you & Justin & Pete like the idea, go for it...


cc @justinfagnani.

@sigmundch
Copy link
Member

Added this to the Later milestone.
Removed Priority-Unassigned label.
Added Priority-Medium label.

@jmesserly
Copy link
Author

Issue #13567 has been merged into this issue.

@sigmundch
Copy link
Member

Added Polymer-P-2 label.

@sigmundch
Copy link
Member

Removed this from the Later milestone.

@sigmundch
Copy link
Member

Removed Polymer-P-2 label.
Added Polymer-Milestone-Later label.

@sigmundch
Copy link
Member

Removed Polymer-Milestone-Later label.
Added PolymerMilestone-Later label.

@sigmundch
Copy link
Member

I've been looking into this while working on rolling the polymer package.

The good news - the hacky code moved out of observe-js and now lives in polymer.
The bad news - the hack of redefining the prototype is still there and is also used for computed properties.

If I understand correctly, here is my understanding of how timing is different here. The logic is differs when there is a binding on a property and when there is not, so here is are details for each case. I am using foo as an example. In JS it represents the public property, and foo_ is the private property (copied from the originally declared foo).

No bindings:
  - js read operation
      - invoke foo getter
      - returns foo_
    (all sync)

  - dart read operation
      - read foo's value
    (this is the only case where Dart and JS match in timing)

  - js write operation
      - invoke foo setter
      - set foo_
      - reflect foo_ value to attribute (if any)
    (all sync)

   - dart write operation
      - set foo
      ... (async break)
      - on change event, reflect to attribute

With a binding on the property:
   - js read operation
       - invoke foo getter
       - invoke bindable.deliver()
          - deliver may set foo_
          - if so, it reflects to attribute too
       - return latest value of foo_
     (all sync)

   - dart read operation
      - read foo's current value
        (which may not include latest changes from bindable)

   - js write operation
      - invoke foo setter
      - set value on bindable
      ... (async break)
      - when bindable delivers, set foo_
      - reflect to attribute
   
    - dart write operation
      - set foo
      ...
      - on change event, set value on bindable
      - also on change event, reflect to attribute
        (which may not be synchronously at the same time as we update bindable).

@sigmundch
Copy link
Member

I decided to go with Pete's suggestion. Part of the reason is that notifications are delivered from the object that contains the property, not the property object itself.

The syntax looks like this:
  @­published
  int get property => readValue(#property, () => 42);
  set property(int value) => writeValue(#property, value);

The change is included in the roll of polymer to match 0.3.4 of polymer.js, which includes also computed properties. See:
  https://codereview.chromium.org/420673002/


Added Started label.

@jmesserly jmesserly added Type-Defect area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. labels Jul 28, 2014
@jmesserly
Copy link
Author

this was landed a while back

@DartBot
Copy link

DartBot commented Jun 5, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams.
Projects
None yet
Development

No branches or pull requests

4 participants