Navigation Menu

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

pkg:observe -- implement dirty-checking fallback for non-observable objects #16342

Closed
jmesserly opened this issue Jan 27, 2014 · 20 comments
Closed
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. closed-not-planned Closed as we don't intend to take action on the reported issue type-enhancement A request for a change that isn't a bug

Comments

@jmesserly
Copy link

After landing https://codereview.chromium.org/132403010/ we'll have most of the infrastructure to do this.

Ideal:

  • Users can get dirty checking for all objects, and faster change notification if the object supports it.

* Ideal: @­observable transform does not require ChangeNotifier base class.

* Need a way of marking an object @­observable in another library.

* Do we need an object to be a ChangeNotifier and have some properties dirty-checked? If so, we need a way of attempting to subscribe, and determining if such a property is not available for subscription. I think we need this to make computed properties really easy to use. Alternatively we can throw an error in this case so user at least knows that they forgot to make a property @­observable.

* remove Observable base class -- objects can be observed via PathObserver/CompoundObserver. Add ObjectObserver from observe-js

* ObservableList still provided as the "fast version"

* Add logging/tracing diagnostics about dirty checking to provide developers a way to assess performance impact of dirty checking -- how many objects, which objects, how often was dirty checking run and how long did it take?

* For best performance, this should be combined with an approach that generates fast-paths for expressions found in HTML

* For now, the @­observable transform will not be run in dev mode, meaning those objects will be treated as dirty-checked. If using "pub serve" the transform will happen automatically.

Any flaws in this plan? We've tried this before and not succeeded. I think the big changes since we last looked at this:

  • pub build is here and pub serve is coming
  • Zones have solved the issue of dirty-checking being triggered unreliably
  • we now understand 'pkg:observe' is a package and does not need to ever be 'dart:observe'
  • mirrors are now acceptable to use and we have other ways of mitigating their performance impact (generate fast-paths for html exprs)
  • this is one of the two reasons users are still stuck on polymer 0.4 (pkg:web_ui). many prefer pkg:web_ui as well as angular.dart for this very reason
@anders-sandholm
Copy link
Contributor

Removed Library-Observe label.
Added Pkg-Observe label.

@jmesserly
Copy link
Author

if we don't do this, we'll need to fix computed properties.

@jmesserly
Copy link
Author

s/fix/make easier ...

@jmesserly
Copy link
Author

Issue #13458 has been merged into this issue.

@DartBot
Copy link

DartBot commented May 28, 2014

This comment was originally written by joo....@gmail.com


yes, list and map work in web_ui fine, but on polymer has not reached the expected results, never.

@jmesserly
Copy link
Author

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

@sigmundch
Copy link
Member

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

@sigmundch
Copy link
Member

Added Polymer-P-2 label.

@sigmundch
Copy link
Member

Removed this from the Later milestone.

@jmesserly
Copy link
Author

IMO, should be higher priority... it's a big usability disadvantage compared with other systems.


Removed Priority-Medium, Polymer-P-2 labels.
Added Priority-High label.

@sethladd
Copy link
Contributor

sethladd commented Jul 8, 2014

High Pri means we have a milestone assigned. Does this mean you're signing up to make it happen in 1.6? :)

@jmesserly
Copy link
Author

@seth -- Sadly no, am not working on this feature area anymore.

@sigmundch
Copy link
Member

Since packages have an independent release cycle than the SDK, I don't want to assign any Milestone (it just confuses everyone).

This is important, there are many other things with higher priority for now, so I doubt we'll get to this that soon.


Added Polymer-P-2 label.

@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.

@sethladd
Copy link
Contributor

sethladd commented Jul 9, 2014

Bumping Priority.


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

@blois
Copy link
Contributor

blois commented Jul 18, 2014

It would be great if there were some way to ensure that dirty-checking does not creep into a codebase unexpectedly. From experience in large XAML apps which had a similar slow-path for non-observables we had to have a strict 'must never use the slowpath' rule, but did not have a decent way of enforcing it.

It's a bit tricky because the final component of a binding path may support observing while intermediate steps do not (or the reverse).

This could potentially be mitigated by tools for inspecting binding state of a DOM tree, but as a dev I'd prefer to be able to prevent this entirely.

@jmesserly
Copy link
Author

Jake/Siggi, does this seem worth it?
if so I can move this to the github tracker, otherwise let's close


cc @jakemac53.
cc @sigmundch.

@jakemac53
Copy link
Contributor

lets close imo

@jmesserly jmesserly added Type-Enhancement area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. closed-not-planned Closed as we don't intend to take action on the reported issue labels Feb 4, 2015
@DartBot
Copy link

DartBot commented Jun 5, 2015

This issue has been moved to dart-archive/observe#58.

@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed type-enhancement labels Mar 1, 2016
This issue was closed.
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. closed-not-planned Closed as we don't intend to take action on the reported issue type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

8 participants