Refactor properties mixins. #13

theengineear · 2019-01-22T05:26:38Z

This refactor attempts to split apart the current /mixins into smaller, more manageable chunks by providing more hooks in properties-mixin.js.

It also adds a new concept called property effects. Using this terminology, property reflection, computation, and observation are all effects that take place based on changes to properties. Note that by splitting these into separate mixins, we can have full control over the order that effects happen.

theengineear · 2019-01-22T18:45:26Z

mixins/computed-properties-mixin.js

+ * Provides observation and computation hooks for properties.
+ */
+
+// TODO: Delay first computation until at least one dependency is defined.


I didn't take on this change here since it would cause a compatibility issue. We should discuss what the appropriate behavior is though.

theengineear · 2019-01-22T18:48:13Z

mixins/property-effects-mixin.js

@@ -0,0 +1,12 @@
+/**
+ * Encodes the order that property effects should take place in.


I think having a compositing layer here is helpful. Note that reflection is treated here separately. This allows us to easily encode that upon an update (1) compute properties that are now invalid (2) call observers (3) reflect properties. I'm happy to debate this order, but I would like to keep these effects as separate files.

theengineear · 2019-01-22T21:17:30Z

test/test-observed-properties.js

@@ -19,21 +19,21 @@ suite('x-element observed properties', async ctx => {
  it(
    'observers are called when properties change',
    JSON.stringify(el.changes) ===
-      '[{"property":"a","newValue":"oh"},{"property":"b","newValue":"hai"},{"property":"c","newValue":"oh hai"},{"property":"b","newValue":"hey","oldValue":"hai"},{"property":"c","newValue":"oh hey","oldValue":"oh hai"}]'
+      '[{"property":"a","newValue":"oh"},{"property":"b","newValue":"hai"},{"property":"c","newValue":"oh hai"},{"property":"c","newValue":"oh hey","oldValue":"oh hai"},{"property":"b","newValue":"hey","oldValue":"hai"}]'


Interesting ordering difference here. Probably worth some additional thought.

theengineear · 2019-01-22T21:18:30Z

mixins/observed-properties-mixin.js

+    static afterPropertyUpdate(target, property, value, oldValue) {
+      super.afterPropertyUpdate(target, property, value, oldValue);
+      if (target[OBSERVERS].has(property) && target[OBSERVERS_READY]) {
+        // TODO: consider changing order of old/new


I would like to change this to be value, oldValue...

theengineear · 2019-01-22T21:39:01Z

mixins/properties-mixin.js

      }
    }

-    get propertiesInitialized() {


I ended up taking this out as it was only being used within this mixin file (as far as i know).

theengineear · 2019-01-22T21:45:49Z

mixins/properties-mixin.js

+      const setter = rawValue => {
+        // TODO: it would be better if we could compare _raw_ values to
+        //  determine whether or not an update was necessary. If that were the
+        //  case, we would call shouldPropertyUpdate _before_ getPropertyUpdate.


This comment is related to the debate of when/how to do type coercion. I still think I fall into the camp of using types for serialization/deserialization only.

theengineear · 2019-01-22T21:47:44Z

mixins/properties-mixin.js

-    ) {
-      const { computed, observer, type, reflect } = definition;
-      const attribute = this.camelToDashCase(property);
+    static addPropertyAccessor(target, property) {


I like how the addPropertyAccessor turned out. Instead of messing with the property descriptors (getter/setter) in all the mixins, the base getter/setter just provides more hooks so that mixins can add to the base behavior.

theengineear · 2019-01-27T03:36:44Z

@klebba this is getting there... still depends on #18, so I'll wait until we get that merged in to bother you about this one.

klebba

Looks great overall, big improvement! Most of my feedback is cosmetic -- I'm guessing when you try to integrate this change in the app you see no difference?

klebba · 2019-01-31T04:45:40Z

etc/topological-sort.js

@@ -0,0 +1,67 @@
+export class CyclicGraphError extends Error {}
+
+function _makeGraph(vertex, mapping, edges, vertices, seen) {


nit: function makeGraph

klebba · 2019-01-31T04:46:11Z

etc/topological-sort.js

+  return { edges, vertices };
+}
+
+function _topologicalSort(edges, visitedEdges, stack, solution) {


nit: function topologicalSort

klebba · 2019-01-31T04:47:20Z

etc/topological-sort.js

+  const solution = [];
+  while (solution.length < graph.vertices.length) {
+    stack.push(graph.vertices.find(v => solution.includes(v) === false));
+    _topologicalSort(graph.edges, visitedEdges, stack, solution);


nit: seems weird to differentiate between these functions with _, maybe pick a better name for one of them

Just a pattern I picked up somewhere when you have a wrapper around a recursive function. But yah, I can certainly find better names.

klebba · 2019-01-31T04:52:48Z

mixins/property-effects-mixin.js

+    }
+
+    static resolveMethodName(target, methodName) {
+      if (target[methodName] instanceof Function) {


we should prioritize static methods here IMO. also I recommend a comment here about how we attempt to call both static and instance level functions

I think I disagree. While we should, by convention, use static methods when we can, I think it's natural that instance methods take precedence over static ones. Either way, yes, I'll leave a comment.

klebba · 2019-01-31T04:55:58Z

mixins/property-effects-mixin.js

+      const dependencyToDependents = {};
+      const dependentToCallback = {};
+
+      for (const [property, definition] of Object.entries(properties)) {


nice es6 fu throughout setupComputedProperties!

klebba · 2019-01-31T05:10:57Z

mixins/property-effects-mixin.js

+      return next;
+    }
+
+    static setupProperties(target, properties) {


Maybe this is a better place to refactor, like:

for (const [property, definition] of Object.entries(properties)) { const p1 = super.setupProperties(target, property, definition); const p2 = this.setupComputedProperties(target, property, p1); this.setupObserverProperties(target, property, p2); }

Not a big deal, just hung up on the repeat iteration of properties in each of these functions.

Yah, I hear you. Getting computed properties set up is a little bit of a pain if we don't loop over everything at once since you need to understand the whole dependency graph.

Willing to do another iteration (pun intended) on this though after we agree on the lifecycles and test suites we have.

klebba · 2019-01-31T05:14:48Z

mixins/property-effects-mixin.js

+      }
+    }
+
+    static propertyDidUpdate(target, property, definition, value, oldValue) {


nit: how about propertyDidChange

6 of one, half a dozen of the other. I actually had that name at one point and then saw that lit-element uses update in a lot of places. I can ~~update~~ change it.

it makes the forceChangeProperty method sound a bit odd imo, but i'll stop griping :)

EDIT: just going to call it changeProperty and drop the force. It will match the fact that we call it render and not forceRender.

klebba · 2019-01-31T05:15:53Z

mixins/property-effects-mixin.js

+      );
+    }
+
+    static serialize(target, property, definition, value) {


nice, much better and simplifies reflection code. would call this serializeProperty

klebba · 2019-01-31T05:16:11Z

mixins/property-effects-mixin.js

+    static propertyDidUpdate(target, property, definition, value, oldValue) {
+      super.propertyDidUpdate(target, property, definition, value, oldValue);
+      if (definition.observer) {
+        // TODO: consider changing order of old/new


nit: don't consider it :)

yep, just changed. the reason i was holding off is that i was hoping to just have a drop-in replacement, but we'll now need a slew of changes when we attempt to use this in the app.

EDIT: on second thought, it sorta seems wack to refactor this and break everything such that we will need a tick-tock release on this. While I'm 100% on board to do this later, I think I'd like to make actual interface changes one by one after this settles for a moment.

klebba · 2019-01-31T05:19:38Z

test/test-computed-properties.js

@@ -10,9 +14,9 @@ suite('x-element computed properties', async ctx => {
    'initialized as expected',
    el.a === undefined &&


strange indent here

klebba

GitHub collapsed properties-mixin.js on me, so I guess I have two pending reviews now!

klebba · 2019-01-31T05:29:46Z

mixins/properties-mixin.js

 const DASH_TO_CAMEL = /-[a-z]/g;
 const CAMEL_TO_DASH = /([A-Z])/g;
-const COMPUTED_REGEX = /^(.+)\((.+)\)$/;
+const PROPERTIES = Symbol.for('__properties__');


nit: is PROPERTY_DEFINITIONS more accurate?

doesn't matter to me. I think this one is a little hard to name since static get properties() is really a mapping of property to property definition. I can change it though (I did actually have that in an earlier change-set and changed my mind).

klebba · 2019-01-31T05:32:13Z

mixins/properties-mixin.js

-        const dependencies = new Map();
-        const resolvers = new Map();
-        const observers = new Map();
+    static initialize(target) {


similar comment from before, can you move for (const [property, definition] of Object.entries(properties)) { ... } into this function and make an atomic initializeProperty(target, property) inner function? it would mean a lot less looping over the same list

Going to do a sweep on these again. It will be a balance between keeping some ephemeral state on the target vs handling it in a closure, I believe.

At any rate, I totally agree and I'll try to fix as best I can.

klebba · 2019-01-31T05:33:15Z

mixins/properties-mixin.js

+        Reflect.deleteProperty(target, property);
+        Reflect.defineProperty(target, property, { get, set, configurable });
+      }
+      target[PROPERTIES_INITIALIZED] = true;


worth a comment on why we have this guard

klebba · 2019-01-31T05:35:24Z

mixins/properties-mixin.js

+      for (const [property, definition] of Object.entries(properties)) {
+        const value = initialValues.get(property);
+        const oldValue = target[property];
+        // TODO: revisit this decision about when to set initial values...


nit: add more info about why

revisited and comment deleted.

klebba · 2019-01-31T05:38:14Z

mixins/properties-mixin.js

-          target[property] =
-            defaultValue instanceof Function ? defaultValue() : defaultValue;
-        }
+    static forceUpdateProperty(target, property, definition, value, oldValue) {


could use a comment about why we need this

klebba · 2019-01-31T05:39:10Z

mixins/properties-mixin.js

-          `Attempted to write "${attr}" as a reflected attribute, ` +
-          `but it is not a Boolean, String, or Number type (${type.name}).`;
-        target.dispatchError(new Error(message));
+    static deserialize(target, property, definition, value) {


nit: would still call this deserializeAttribute

sure. it feels funny since attribute is not passed as an argument, but I can change.

klebba · 2019-01-31T06:36:40Z

I tried to drop this into a project and noticed that the attribute deserialization was not working, strings were not being coerced to the Number type. Probably a sign that my prior tests are too sparse

theengineear · 2019-02-01T08:34:36Z

@klebba thanks for the review! I've only given it a quick look so far, but it all seems reasonable. I noticed that I had introduced a bug in #18 , so I spent time fixing that and upping our testing game there. If I'm feeling it, I may circle back to this over the weekend. We'll see ;)

theengineear · 2019-02-03T02:16:21Z

I tried to drop this into a project and noticed that the attribute deserialization was not working, strings were not being coerced to the Number type. Probably a sign that my prior tests are too sparse

Yah, I haven't done a full round of integration testing on this yet, so I'm not surprised I've missed some stuff. I'll let you know when my confidence in the change goes up 👍

theengineear · 2019-02-03T04:51:03Z

@klebba another iteration is up. I think I got to most of your concerns. I still feel like the code is less than elegant though...

Notably, this should all happen as expected, synchronously (emphasis on synchronous!):

Properties are analyzed during construction
Computations happen only after connectedCallback
The render method is called synchronously after computations.
Reflection & Observation happens.

1. Postpone more work until initial connect (closes #14 & closes #22). 2. Upgrade all properties at initialization time (closes #15). 3. Initialize properties before initial render (closes #17). 4. Improve handling of property effects (closes #19). 3. Robustify test suite.

theengineear · 2019-03-08T08:32:15Z

@klebba this PR is getting pretty long/gross. I would like to invite others to comment, so I'm going to close it and create a new PR >> #23

theengineear requested a review from klebba January 22, 2019 05:26

theengineear force-pushed the refactor-properties-mixins branch from 0d91477 to 83913f0 Compare January 22, 2019 18:43

theengineear commented Jan 22, 2019

View reviewed changes

theengineear force-pushed the refactor-properties-mixins branch 2 times, most recently from f0e89e1 to d0ac9b3 Compare January 27, 2019 03:35

klebba reviewed Jan 31, 2019

View reviewed changes

theengineear force-pushed the refactor-properties-mixins branch from d0ac9b3 to 1b4c8ed Compare February 3, 2019 04:49

theengineear force-pushed the refactor-properties-mixins branch from 1b4c8ed to d684e63 Compare March 8, 2019 08:28

theengineear mentioned this pull request Mar 8, 2019

Improve initialization #18

Closed

theengineear closed this Mar 8, 2019

theengineear deleted the refactor-properties-mixins branch March 8, 2019 08:32

theengineear restored the refactor-properties-mixins branch March 8, 2019 08:32

theengineear mentioned this pull request May 7, 2019

Cache static analysis per-class and reuse during initialization. #28

Closed

		@@ -0,0 +1,12 @@
		/**
		* Encodes the order that property effects should take place in.

		@@ -0,0 +1,67 @@
		export class CyclicGraphError extends Error {}

		function _makeGraph(vertex, mapping, edges, vertices, seen) {

		@@ -10,9 +14,9 @@ suite('x-element computed properties', async ctx => {
		'initialized as expected',
		el.a === undefined &&

Refactor properties mixins. #13

Refactor properties mixins. #13

Conversation

theengineear commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theengineear commented Jan 27, 2019

klebba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theengineear Feb 3, 2019 • edited Loading

Choose a reason for hiding this comment

klebba Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theengineear Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klebba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klebba commented Jan 31, 2019

theengineear commented Feb 1, 2019

theengineear commented Feb 3, 2019

theengineear commented Feb 3, 2019 • edited Loading

theengineear commented Mar 8, 2019 • edited Loading

theengineear Feb 3, 2019 •

edited

Loading

klebba Jan 31, 2019 •

edited

Loading

theengineear Mar 8, 2019 •

edited

Loading

theengineear commented Feb 3, 2019 •

edited

Loading

theengineear commented Mar 8, 2019 •

edited

Loading