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

Proposal: property definition includes binding dependency setup #295

Closed
James0x57 opened this Issue Dec 13, 2017 · 18 comments

Comments

Projects
None yet
3 participants
@James0x57
Copy link

James0x57 commented Dec 13, 2017

Purpose:

  • self-contained, complete, definition of a property that depends on or tracks other properties
  • provide a clean and safe alternative to deliberate side-effects
  • reduces imperative code within a ViewModel while still maintaining a familiar OO external API

Implementation:

  • Allow a connected function on the definition of a property in a DefineMap.
    • Named connected because can-component's connectedCallback and the related web spec.
  • The developer would set up this.listenTo bindings in the function.
  • A resolve function is passed to the connected function
    • the property's value is set with the resolve function

Common uses of deliberate side-effects and example code:

How its done nowHow it could be done
1. list component with toggleable inline editing of the selected list item, when a different one is selected, cancel editing
    isEditing: "boolean",
    selectedItem: {
      Type: Item,
      set: function (newVal) {
        this.isEditing = false;
        return newVal;
      }
    }
    isEditing: {
      type: "boolean",
      connected (resolve) {
        this.listensTo("selectedItem", () => resolve(false))
      }
    },
    selectedItem: Item
2. category and subcategory dropdowns. Reset the selected subcategory if the category changes.
    categoryId: {
      type: "number",
      set: function () {
        this.subCatId = null;
      }
    },
    subCatId: "number"
    categoryId: "number",
    subCatId: {
      type: "number",
      connected (resolve) {
        this.listensTo("categoryId", () => resolve(null))
      }
    }
3. Mutually exclusive booleans. PropA should become false when PropB becomes true.
    isRed: {
      type: 'boolean',
      set: function (newVal) {
        if (newVal) {
          this.isYellow = false;
          this.isGreen = false;
        }
        return newVal;
      },
      value: false
    },
    isYellow: {
      type: 'boolean',
      set: function (newVal) {
        if (newVal) {
          this.isRed = false;
          this.isGreen = false;
        }
        return newVal;
      },
      value: false
    },
    isGreen: {
      type: 'boolean',
      set: function (newVal) {
        if (newVal) {
          this.isRed = false;
          this.isYellow = false;
        }
        return newVal;
      },
      value: true
    }
    isRed: {
      type: "boolean",
      value: false,
      connected (resolve) {
        const falseIfTrue = newVal => newVal && resolve(false);
        this.listensTo("isYellow", falseIfTrue);
        this.listensTo("isGreen", falseIfTrue);
      }
    },
    isYellow: {
      type: "boolean",
      value: false,
      connected (resolve) {
        const falseIfTrue = newVal => newVal && resolve(false);
        this.listensTo("isRed", falseIfTrue);
        this.listensTo("isGreen", falseIfTrue);
      }
    },
    isGreen: {
      type: "boolean",
      value: true,
      connected (resolve) {
        const falseIfTrue = newVal => newVal && resolve(false);
        this.listensTo("isRed", falseIfTrue);
        this.listensTo("isYellow", falseIfTrue);
      }
    },
4. Counting changes of another property
    nameChanged: "number",
    name: {
      type: "string",
      set: function (newVal, lastSetVal) {
        this.nameChanged++;
        return newVal;
      }
    }
    name: "string",
    nameChanged: {
      connected (resolve) {
        this.listensTo("name", function () {
          resolve( this.nameChanged + 1 );
        });
      }
    }
@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Dec 13, 2017

I think type should be ignored to be consistent with other “getters”.

@matthewp

This comment has been minimized.

Copy link
Contributor

matthewp commented Dec 13, 2017

When is connected called?

@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Dec 13, 2017

@matthewp

This comment has been minimized.

Copy link
Contributor

matthewp commented Dec 13, 2017

What are the rules behind which behaviors conflict with either other? For example, can you have both a connected and a set? A connected and a get? I would think not, but it would be good to clarify this some how, and provide warnings if conflicting behaviors are used together.

@James0x57

This comment has been minimized.

Copy link
Author

James0x57 commented Dec 14, 2017

My assumption was that when it resolve( ... )s with a value that it would be like setValue in async getters and set the value. So it would still be compatible with get and set if that was needed.
Though you should probably use streams if it's that involved

@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Dec 14, 2017

Async getters are getters themselves so they can’t be used with other getters. If you’d like to to work with set, we need a mechanism for how set values are made available to connected.

@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Dec 20, 2017

A few additional thoughts:

Using the absence of resolve to indicate that an unbound value is being read.

Say I wanted to make fullName work even if someone hasn't bound to it:

fullName: {
  connected: function(resolve){
    if(!resolve) {
      return this.first + " " + this.last;
    } else {
      this.listenTo("first", ()=> {
        resolve( this.first + " "  + this.last );
      })
      this.listenTo("first", ()=> {
        resolve( this.first + " "  + this.last );
      })
      return this.first + " " + this.last;
    }
  }
}

Pass listenTo

I think technically, it might be better if listenTo was passed to connected like:

connected (resolve, listenTo) {
    listensTo("name", function () {
        resolve( this.nameChanged + 1 );
    });
}

This makes it easier to track the bindings for that property. That way we can undo just those bindings when that property becomes undefined.

Also in situations where a value is read without binding, we DON'T want people to call this.listenTo. The listenTo argument would also be undefined:

fullName: {
  connected: function(resolve, listenTo){
    if(listenTo) {
      listenTo("first", ()=> {
        resolve( this.first + " "  + this.last );
      })
      listenTo("first", ()=> {
        resolve( this.first + " "  + this.last );
      })
    }
    return this.first + " " + this.last;
  }
}

Oh, one more reason ... this listenTo should default to listening in the notify queue.

Pass a lastSet observable?

If we want to support setting the value, we should probably pass the last set value as a simple observable. For example, the following would use the value that lastSet was set to, otherwise, it would derive from first and last:

fullName: {
  connected: function(resolve, listenTo, lastSet) {
     if(listenTo) {
      listenTo(lastSet, resolve);
      listenTo("first", ()=> {
        resolve( this.first + " "  + this.last );
      })
      listenTo("first", ()=> {
        resolve( this.first + " "  + this.last );
      })
    }
    return lastSet.get() ? lastSet.get() : this.first + " " + this.last;
  }
}

NOTE: what you'd really want to do here is have setting fullName actually set first and last. Though this breaks encapsulating the mutability. I'm not sure how first and last could listen to the "internal" setting of fullName. Perhaps here is where can-reducer is really needed.

Alternatively, setting fullName would only ever produce an event like setFullName that first and last would listen to and update their values.

@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Jan 10, 2018

So I was trying to solve the fullName that works as a getter / setter ... and there's a problem. First, here's how I implemented it:

  • first listens to itself being set and fullName being set
  • last listens to itself being set and fullName being set
  • fullName listens to first and last and updates itself

NOTE: when a property with a value resolver is set, an event named propertyName+"Set" is dispatched.

var Person = DefineMap.extend("Person", {
        first: {
            value: function(resolve, listenTo){
                listenTo("firstSet", function(ev, newVal){
                    resolve(newVal);
                });
                listenTo("fullNameSet", function(ev, newVal){
                    var parts = newVal.split(" ");
                    resolve(parts[0]);
                });
            }
        },
        last: {
            value: function(resolve, listenTo){
                listenTo("firstSet", function(ev, newVal){
                    resolve(newVal);
                });
                listenTo("fullNameSet", function(ev, newVal){
                    var parts = newVal.split(" ");
                    resolve(parts[0]);
                });
            }
        },
        fullName: {
            value: function(resolve, listenTo){
                var first = this.first,
                    last = this.last;
                resolve(first + " " + last);
                listenTo("first", function(ev, newFirst){
                    first = newFirst;
                    resolve(first + " " + last);
                });
                listenTo("last", function(ev, newLast){
                    last = newLast;
                    resolve(first + " " + last);
                });
            }
        }
    });

The problem here is that new Person({first: "Justin", last: "Meyer"}) doesn't actually retain the value because first and last are not bound during construction of the instance.

Possible fixes:

  • When setting a value, the underlying observation will be temporarily bound, calling its resolver function. Then the set event is dispatched. If resolve is called ... that value is retained. If the value is retained; however, this cause problems with other situations. We might only "retain" a value after a set.

  • Ignore "setting" right now. This sort of bi-directional relationship isn't really needed in most applications.

@matthewp

This comment has been minimized.

Copy link
Contributor

matthewp commented Jan 10, 2018

Some things I don't like/understand:

  1. Why you would need to have "Set" in the thing you are listening to. I don't understand why first can't listen to "fullName" instead. Cycles?
  2. I guess that's really the source of my confusion so maybe more questions after.
@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Jan 10, 2018

@matthewp cycles is part of it, but lets say you wanted to mimic a setter that took a number and rounded it:

integer: {
  value(resolve, listenTo ) {
    listenTo("integerSet",  ( ev , newVal ) => {
      resolve( Math.round(newVal) )
    })
  }
}

We need to distinguish the integer event, which is emitted with "rounded" numbers, and the the event when the setter is called, which is emitted with the raw set value.

@matthewp

This comment has been minimized.

Copy link
Contributor

matthewp commented Jan 10, 2018

Why do we need to distinguish between the two?

@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Jan 10, 2018

@matthewp if we don't how could integer be implemented otherwise? If some other property listened to integer events, they would see rounded and un-rounded numbers ... that wouldn't be right.

@matthewp

This comment has been minimized.

Copy link
Contributor

matthewp commented Jan 11, 2018

Ah, ok, it seems like its not a great use-case for listenTo then. I think the cognitive load of knowing how a property changed is too much. Not when listening to yourself maybe, but when listening to others.

@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Jan 11, 2018

Random notes from a convo with @matthewp

fullName: {
    get() {
        return this.first + this.last
    },
    set(fullName){
        var parts = fullName.split(" ");
        this.first = parts[0]
        this.last = parts[1]
    }
},
first: {

}


open: [],
closed: []


isOpen: {
    open: function(){ return true },
    closed: function(return false)
}


open: function(){
    this.dispatch("open")
}


isOpen: {
    stream: function(){
        this.stream("open").merge(this.stream("closed")).scan(function(event){
            return event.type === "open" ? true : false
        })
    }
}

age: {
    
    get(lastSet) {
        return Math.age( lastSet.get() )
    }
}


reborn: {
    
}

person.age = "32.4"
person.age //-> 32.4

age: {
    value(resolve, on, off, lastSetValue) {
        on(lastSetValue,resolve);
    }
},
ageInteger: {
    value() {
        listenTo("age", function(ev, age){
            resolve(Math.round(age))
        })
    }
}
@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Jan 12, 2018

@matthewp So I'm trying to replicate a mini make-model-year example with the city/state selector from the Getter limitations section here: https://canjs.github.io/next/doc/can-define/map/map.html#Declarativeproperties :

Locator = DefineMap.extend({
    state: {
        type: "string",
        set: function(){
            this.city = null;
        }
    },
    city: "string"
});

var locator = new Locator({
    state: "IL",
    city: "Chicago"
});

locator.state = "CA";
locator.city //-> null;

Without providing setter support, it will look like:

var Locator = DefineMap.extend("Locator",{
	state: "string",
    setCity: function(city){
        this.dispatch("citySet",city);
    },
	city: {
        value: function(resolve, listenTo) {
            listenTo("citySet", function(ev, city){
                resolve(city);
            });
            listenTo("state", function(){
                resolve(null);
            });
        }
    }
});

var locator = new Locator({
	state: "IL"
});
locator.on("city", function(){});

locator.setCity("Chicago");

locator.state = "CA";
QUnit.equal(locator.city, null, "changing the state sets the city");

NOTICE That we can't initialize locator with a city. This seems like a big problem for components where you want to be able to do: <locator city:bind="blah"/>.

So, I am now leaning to having a fourth lastSet observable argument, making this example look like:

var Locator = DefineMap.extend("Locator",{
	state: "string",
	city: {
        value: function(resolve, listenTo, stop, lastSet) {
            listenTo(lastSet, function(city){
                resolve(city);
            });
            listenTo("state", function(){
                resolve(null);
            });
            resolve( lastSet.get() );
        }
    }
});

var locator = new Locator({
	state: "IL",
    city: "Chicago"
});
locator.on("city", function(){});

locator.state = "CA";
QUnit.equal(locator.city, null, "changing the state sets the city");

The good thing about this is that I can probably make it so city can be read even if unbound. It just doesn't change its value until its bound.

cc @phillipskevin @chasenlehara

@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Jan 12, 2018

Instead of 4 arguments, we could do a property object with those methods:

var Locator = DefineMap.extend("Locator",{
	state: "string",
	city: {
        value: function(property) {
            property.listenTo(property.lastSet, function(city){
                property.resolve(city);
            });
            property.listenTo("state", function(){
                property.resolve(null);
            });
            property.resolve( lastSet.get() );
        }
    }
});

What I like about this is that we can create additional helpers like merge:

isCardInvalid: {
  value(property) {
    property.listenTo( property.merge(["cardError","expiryError","cvcError"], function(ev, val){
      if(ev.type === "BLAH") {
        return ...
      }
    }, false), resolve )
  } 
}
@matthewp

This comment has been minimized.

Copy link
Contributor

matthewp commented Jan 12, 2018

I'm warmer on this when listening lastSet. I still feel that listening to special internal strings is too much cognitive overhead.

As for 4 arg vs property, I think we should preserve the 2 arg form of (resolve, listenTo) since I think that will cover the common cases why you might use this feature. I think maybe you mentioned an idea of providing a different context to the function by creating an object with the instance set as its prototype. I kind of like that. You could add your .merge and other stuff there.

@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Jan 12, 2018

@matthewp I'm leaning to property as shown here:

var Locator = DefineMap.extend("Locator",{
	state: "string",
	city: {
        value: function(property) {
            property.listenTo(property.lastSet, function(city){
                property.resolve(city);
            });
            property.listenTo("state", function(){
                property.resolve(null);
            });
            property.resolve( lastSet.get() );
        }
    }
});

There's a few reasons:

  1. having property means that people w/o arrow functions ()=>{} won't have to worry about making it available with something like var self = this.
  2. All other property definition behaviors have this as the instance.
  3. property.resolve is reasonable semantic compared to this.resolve(), especially with this normally being the instance.
  4. I can make property the actual ResolverObservable instance. It makes documentation a bit more straightforward as I can just doc the argument to value like:
@param {can-simple-observable/resolver/resolver} property An instance of a "Resolver" compute.  You can use `property.listenTo`, `property.stopListening`, blah blah blah.

This will automatically link to the resolver docs. Then if we add features to resolver, they only have to be documented in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.