Properties rewrite proposal

[johnspackman]

This is a proposal for a complete rewrite of the properties mechanism in Qooxdoo classes, it is 100% BC and adds a number of really useful features; a big motivation for this complete rewrite is that the previous system relied on code generation, which was great for IE6 but in modern JS engines this increases memory usage and circumvents JIT compiler technology. Worse of all, the code generation code has grown to be virtually unmaintainable.

The second big motivation is to improve the ease of use, both when defining properties (simpler specification boilerplate) and using them (native properties), and to add better type checking and control over storage and other semantics.

Finally, this is to allow weak references to be built into Qooxdoo in a way that is transparent and can be managed automatically with garbage collection.

New Property Features:

• Native properties
• Properties are first-class objects
• Storage can be completely replaced
• Eliminate the need for psuedo-properties
• Eliminate the need for property sniffing/detection
• Support for private and protected properties
• More advanced, and reusable, type checking
• Immutability and mutation detection
• Fast property definition
• Integration with References

References are included in this PR because property support for References is integral to making References transparent and useful. Reference features are:

• Hard, Weak, Soft, and On Demand
• Transparent, asynchronous, object destructors and garbage collection

Native properties

Add native property getters/setters so that it is possible to use myObject.someValue instead of myObject.getSomeValue() / myObject.setSomeValue(…).

Compiler Changes

This should also work for arrays backed by qx.data.Array, so that myObject.getMyArray().getItem(0) becomes myObject.myArray[0] but this requires compiler changes to translate all array references into a function, eg qx.data.Array.get(myObject.myArray, 0) which tests for the type of myObject.myArray

Eliminate Psuedo Properties

There are lots of places where objects have to define something that looks like a property, but does not list it in the properties section. The most common example is usually because the property stores an instance of qx.data.Array, and when you call object.setArrayProperty() with a new value, the widget only wants to change the contents of the array and does not want to change the actual instance of the array.

For example, see qx.ui.form.MModelSelection and the modelSelection property.

What MModelSelection.js has to do is define a setModelSelection, getModelSelection, resetModelSelection and an event called "changeModelSelection" - it must define all four because otherwise the binding will fail. IMHO this is poor because it's not clear from MModelSelection.js whats going on, plus it's reproducing all the existing property code, and it makes it impossible for the framework to add features without declaring that class as broken.

There are two ways to solve this in this plan, one is to override the storage, maybe something like this:

properties: {
  modelSelection: {
    init: () => new qx.data.Array(),
    get: () => this.$$user_modelSelection,
    set: value => {
      if (!this.$$user_modelSelection)
        this.$$user_modelSelection = value;
      else
        this.$$user_modelSelection.replace(value||[]);
    }
  }
}

or because arrays are a common usage, to bake it into the property mechanism, eg this version:

properties: {
  modelSelection: {
    init: () => new qx.data.Array(),
    immutable: "replace"
  }
}

This would be properly defined, so that immutable: "replace" only works on objects which implement some interface eg qx.data.IReplacable, and qx.data.Array would implement that interface.

When the developer tries to set the value of modelSelection, the items are copied into the existing modelSelection array.

You can get the modelSelection property and change it directly, and that's part of the reason for this kind of pattern. If you are reacting to changes in the modelSelection array, you would normally have to listen for "changeModelSelection" to know when the property value changes, and then add/remove listeners for the "change" event for old and new values, and sync the array up to whatever you're listening to it for.

It's basically a boat load of extra hard work (and it can get exponential), and so by simply saying "ok this property value will never change, just it's contents" you make life massively easier, without sacrificing compatibility or the dynamics of binding.

Because properties are not always defined in the properties section, any code (eg the binding code) cannot rely on reflection to get properties, it has to do it by sniffing.

This impacts storage and immutability

Storage

Properties must be able to override the storage mechanism, either by providing your own get, set etc or by specifying that it is held by reference (and to be able to choose the reference implementation).

The minimum required would be get and set, all others (eg reset etc) would have default implementations

Immutable Values

Support properties where the value stored is locked; for scalar values, this could mean that the property is read only and attempts to change it are ignored or an exception is raised, for objects like qx.data.Array, this would mean that the instance never changes but the contents of the array are changed:

arrayProp: {
  init: () => new qx.data.Array(),
  immutable: "replace"
}

Where immutable is one of [ null, “replace”, “readOnly” ]

var array = myObject.getArrayProp(); // instanceof qx.data.Array
var tmp = new qx.data.Array();
tmp.add(1);
tmp.add("two");
myObject.setArrayProp(tmp);
qx.core.Assert.assertTrue(array === myObject.getArrayProp());
qx.core.Assert.assertTrue(tmp !== myObject.getArrayProp());
qx.core.Assert.assertTrue(tmp.equals(array));
myObject.setArrayProp(null);
qx.core.Assert.assertTrue(array === myObject.getArrayProp());
qx.core.Assert.assertTrue(myObject.getArrayProp().getLength() === 0);

Detecting Mutation

Check whether a property is being changed eg

if (myObject.isPropertyMutating(“myProp")) { 
  await myObject.resolveMutation(“myProp"); 
}

(Or fire “beforeChangeXxx” and “afterChangeXxx” ??)

First-Class Implementation

Each property should be backed by a first class object, which has .set(object, value), .get(object), .check(value) etc methods. It should be simple to obtain these instances, eg myObject.constructor.getProperty(“someValue”).get() is the same as myObject.getSomeValue().

The qx.Class definition will detect existing pseudo-properties and warn that it is deprecated to use pseudo-properties but wrap the methods with first class property objects in the mean time, so that property reflection is accurate.

A separate project would be to migrate the binding and other classes over to this mechanism.

Redefining

Currently there are only limited options for redefining a property, this will be expanded - specifically, type checks will be able to be further constrained in derived classes

Type Checking

Type checking will become more advanced expression, the same kind of expression that we use for JSDOC - it will include support for parameterised types (eg "qx.data.Array<MyClass>"), logical OR “|”, and indicate nullability with a trailing ?

Checks will be implemented as a separate, first-class object because this will allow the check code to be used. The checks will follow JSDoc so that, in the future, the compiler could be augmented to copy the JSDoc into type-checking code inserted into the method for parameters and return types.

Deprecate nullable

Whether a property is nullable or not becomes part of the type checking - initially we will support both, and will provide a tool (similar to qx es6ify) to migrate nullable into a check. It will only throw an error if there is a conflict, eg check: "Boolean?", nullable: false would not be allowed

Private and Protected Properties

Protected and private properties will be supported, with mangling for private properties.

I don't know what happens to properties with "_" and "__" prefixes at the moment - but if it turns out that you end up with methods like get_myProperty, then this will be deprecated in exchange for _getMyProperty etc.

Fast property definition

Many properties are very similar in that they have a check, an event, an optional apply method, and the event name at least follows a strict rule of "changePropertyName" - we know that rule is adhered to because the binding requires it. With nullability included in the type check expression, we can provide an optional "fast" definition of just the type definition:

properties: {
  name: "String?",
  address: "String?",
  children: {
    init: () => new qx.data.Array(),
    check: "qx.data.Array<mypkg.Person>",
    immutable: "replace"
  }
},

members: {
  _applyName(value, oldValue) {
    /* ... snip... */
  }
}

would be the same as:

properties: {
  name: {
    init: null,
    check: "String?",
    event: "changeName",
    apply: "_applyName"
  },
  address: {
    init: null,
    check: "String?",
    event: "changeAddress"
  },
  children: {
    init: () => new qx.data.Array(),
    check: "qx.data.Array<mypkg.Person>",
    immutable: "replace"
  }
},

members: {
  _applyName(value, oldValue) {
    /* ... snip... */
  }
}

In this example, note that the event name is assumed and the apply method is auto-detected.

Event Names

Every property will have an event name, unless you specify event: null. You can manually provide an event name - although it will be a warning to use an event name which is not in the form "changePropertyName"

Asynchronous properties

The tough issue with using asynchronous properties is that you have to await for every stage, and there isn't any clear indication that you need to - myObject.myArrayProperty[2].myOtherProperty looks like a normal expression (with native properties), but what if myArrayProperty was defined as asynchronous, and on demand?

If myArrayProperty was already known, then the getter can return immediately (ie synchronously) but that makes for unpredictable behaviour unless you can guarantee that you fetch the value (with await myObject.getMyArrayPropertyAsync()) earlier in the code.

Another approach would be to use bindings, except that although bindings do not currently support async properties at all, this is something that could change.

There is a possible work around where a call to an asynchronous function can be made to be synchronous, outlined here in web workers: https://www.smashingmagazine.com/2022/04/partytown-eliminates-website-bloat-third-party-apps/

Init

init property to accept a function that is called to create the init value; if init is null, then assume nullable:true (or a nullable type check) unless explicitly assigned

Add the ability to check for an uninitialised properties without triggering an exception because the property is “not yet ready”

Other

apply to support functions instead of strings

References

All Qooxdoo objects stored in properties will be tracked via a Reference class, which can be a HardReference, WeakReference, or OnDemandReference.

WeakReferences will be backed by WeakRef which is now in all browsers and nodejs since v14

On Demand

OnDemandReferences have a URI that can be used to load an object on demand, and internally use a Hard/Soft/Weak Reference to store the value in the mean time.

This means that there will be some kind of LifecycleManager for different classes as objects come and go

Examples

These are not hard and fast, just some ideas:

myProperty: {
    get: (obj) => obj.$$user_myProperty,
    set: (obj, value) => obj.$$user_myProperty = value
}

myArray: {    
    check: “qx.data.Array<qx.core.Object>”,
    init: () => new qx.data.Array()
}

[derrell]

@johnspackman, this looks great! I am still digesting it, but have a few comments/questions in the meantime:

On Mon, Jun 6, 2022 at 4:15 PM John Spackman @.***> wrote:

Eliminate Psuedo Properties

Many classes need to roll their own property implementation (eg the selection arrays of various widgets), meaning that changes to the property features would have an impact on those classes; any code (eg the binding code) cannot rely on reflection to get properties, it has to do it by sniffing.

Can you be more specific about what these pseudo properties are? It doesn't sound like you're referring to "group" properties.

Immutable Values

Support properties where the value stored is locked; for scalar values, this could mean that the property is read only and attempts to change it are ignored or an exception is raised, for objects like qx.data.Array, this would mean that the instance never changes but the contents of the array are changed:

arrayProp: {

init: new qx.data.Array(),

immutable: "replace"

}

Where immutable is one of [ null, “replace”, “readOnly” ]

var array = myObject.getArrayProp(); // instanceof qx.data.Array

var tmp = new qx.data.Array();

tmp.add(1);

tmp.add("two");

myObject.setArrayProp(tmp);

qx.core.Assert.assertTrue(array === myObject.getArrayProp());

qx.core.Assert.assertTrue(tmp !== myObject.getArrayProp());

qx.core.Assert.assertTrue(tmp.equals(array));

myObject.setArrayProp(null);

qx.core.Assert.assertTrue(array === myObject.getArrayProp());

I wonder about the performance of this. It looks like there is copying involved. Why not just have the developer obtain the single reference and make the changes directly in the array? And as shown above, does myObject.setArrayProp replace the entire contents of the property's array with the contents of the new array, or is there a provision for merging, e.g., if the two arrays are not the same size. I'm thinking about Table's data model, where individual rows are changed and then the table is updated.

Detecting Mutation

Check whether a property is being changed eg if (myObject.isPropertyMutating(“myProp")) { await myObject.resolveMutation(“myProp"); }

(Or fire “beforeChangeXxx” and “afterChangeXxx” ??)

I assume this is for long-lasting, async property changes?

First-Class Implementation

Each property should be backed by a first class object, which has .set(object, value), .get(object), .check(value) etc methods. It should be simple to obtain these instances, eg myObject.constructor.getProperty(“someValue”).get() is the same as myObject.getSomeValue().

I assume that was intended to be myObject.getProperty("somevalue").get() (without constructor) so that it obtains the instance's property rather than the class' property (if class properties even existed), right?

The qx.Class definition will detect existing pseudo-properties and warn that it is deprecated to use pseudo-properties (but wrap the methods with first class property objects in the mean time)

I still don't understand this. I think I need an example of a pseudo-property.

Type Checking

Type checking will become more advanced expression, the same kind of expression that we use for JSDOC - it will include support for parameterised types (eg "qx.data.Array"), logical OR “|”, and indicate nullability with a trailing ?

Checks will be implemented as a separate, first-class object because this will allow the check code to be used. The checks will follow JSDoc so that, in the future, the compiler could be augmented to automatically output type-checking for parameters and return types in source targets.

I have no objection to adding to the capabilities of strings used as the value of a check constraint. My preference, though, is to use functions for this purpose.

Private and Protected Properties

Will be possible for protected and private properties to be declared, with mangling for private properties.

My idea for this eliminates mangling for new private variables (uses the #varName capability). Manging will still be used for traditional, pseudo-private (__ prefixed) variables.

Fast property definition

Many properties are very similar in that they have a check, an event, an optional apply method, and the naming follows a strict set of rules - we know that the rules are adhered to because the binding requires it. With nullability included in the type check expression, we can provide an optional "fast" definition...

I'm on the fence about this, mostly because I have different opinions about what the defaults are (e.g., I wouldn't expect an apply function to be part of a "fast" property definition ), and I suspect it will cause more problems than it solves (e.g., neglecting to create that apply member method). My preference would be to elide this new feature. I don't think it's of sufficient value to be worth the problems it could create.

Event Names

Every property will have an event, unless you specify event: null. You can manually provide an event name - although it will be a warning to use an event name which is not in the form "changePropertyName"

If a property has an event, there is code that will be run every time the property value changes, to invoke the event listeners, even if there are no listeners. (It calls hasEventListeners() or some such thing.) My experience is that events are used for properties only about 50% of the time. I think, for the small benefit to the developer of not having to specify the event name in the property definition, the added performance cost is probably not a good tradeoff.

Init

init property to accept a function that is called to create the init value; if init is null, then assume nullable:true (or a nullable type check) unless explicitly assigned

It is not uncommon for properties to hold functions as their values. There are places in the framework that do so. We would need a way to distinguish between a function as the init value being used to provide the actual init value, and the function being the intended value itself.

References

All Qooxdoo objects stored in properties will be tracked via a Reference class, which can be a HardReference, SoftReference, WeakReference, or OnDemandReference.

The difference, as I've seen, between a "soft" and "weak" reference (in Java) is that a "soft" reference delays the actual garbage collection of the object for some period of time. In JavaScript, we have no control over exactly when an object is garbage collected. What do you see as the difference here between "soft" and "weak" references?

On Demand

OnDemandReferences have a URI that can be used to load an object on demand, and internally use a Hard/Soft/Weak Reference to store the value in the mean time.

I don't understand this at all. Would you please elaborate on what an OnDemandReference is, what is this URI, and how is it used?

Garbage Collection will be implemented to allow for WeakReferences (starter for 10: https://github.com/johnspackman/qx-gc)

That implementation is 7 years old and predates the work done more recently in qooxdoo to reduce the need for manually calling dispose(). Is it still necessary?

Message ID: @.***>

[johnspackman]

Psuedo-properties - The description above is updated

or is there a provision for merging, e.g., if the two arrays are not the same size

No, this is just about replacing. Ultimately, the semantics of someWidget.setModelSelection(myNewArray) is "replace the existing array with myNewArray`, not merge or anything complex.

Type Checking

I have no objection to adding to the capabilities of strings used as the value of a check constraint. My preference, though, is to use functions

No problem, this isn't about eliminating functions but string expressions are about adding a lot of sophisticated checks that can compared.

For example, if you were to say this:

properties: {
  children: {
    check: "qx.data.Array<mypkg.Person>"
  }
}

The check can ensure that each element in that children array is an instance of mypkg.Person.

Fast Property Definition

I'm on the fence about this... I suspect it will cause more problems than it solves (e.g., neglecting to create that apply member method)

The idea is that it detects if there is a method with an appropriate name and uses it only if there is one.

I find my self writing so much boiler plate for property after property, admittedly that's more significant when building a new model for a new app, but so many things are just obvious. Eg why do I have to say nullable: true and init: null? Why do I have to specify the event name when it is mandatory?

Event Names

If a property has an event, there is code that will be run every time the property value changes

I would agree except that my experience of tracking through the event firing code is that it has been pretty well written to preserve performance and do very little if nothing is required - so firstly I wonder how much of a performance impact this will have.

I could imagine that on some of the very performant parts of the framework (such as UI properties which change very fast) there would be cause to disable it (eg by setting event: null), but most properties change very infrequently, and any business object generally needs to have everything bindable.

This comes from writing pages and pages of property definitions lately, and every name is 100% predictable.

Init

It is not uncommon for properties to hold functions as their values.

Yes, now I agree that this is tricky to implement and could actually introduce BC problems.

The problem as I see it is that any non-scalar value needs to be separately initialised in the constructor, miles away from the property definition, and this is not helpful for documentation or flow.

Functions could be provided as values like this:

properties: {
  callback: {
    init: () => function() { /* do something */ },
    check: "Function"
  }
}

References Soft vs Weak references are all about how soon a referenced object is garbage collected; I'm not sure that there is a specific definition outside the implementation of a specific garbage collector, but the idea is that Weak disappears quickly (proactively) and Soft disappears when memory is needed for other things.

That qx-gc is only a single generation mark and sweep collector, so weak refs would be easy but would need some scaling for soft refs.

My plan was to implement GC and everything manually, based on the qx-gc contrib I tried years ago - it worked quite well but I couldnt retrofit it into Qooxdoo at the time. The automatic memory management got rid of most common dispose issues, and was a lot less intrusive.

However, I've seen now that WeakRef is supported by NodeJS and modern browsers, so I am questioning whether I will be doing a GC - there is still some things I need to think about here, particularly because WeakRefs don't have any disposal.

This stuff is different to the memory management for .dispose (which was for the majority of client-only use cases) but is really important for for long running systems with heavy caching and object sharing, eg when many many clients are connecting to a server, or there are large object graphs that are populated by separate pieces of code and there needs to be a rational way of managing it all.

On Demand On demand is a specialised kind of reference that would itself use a WeakRef or hard reference to hold whatever value is obtained. In order to obtain something on demand, there needs to be some kind of identifier that the OnDemandReference class can use, and so I imagine a URI.

With the addition of some kind of OnDemandLifecycleManager, you would be able to register a class that can fetch for a given URI scheme.

So for example, when restoring an object from a database or a server network connection, any pointers that object has to other objects could be restored as a URI; and only when the code actually needs to get those other objects is the object actually fetched from the database

With the OnDemandReference backed by a WeakRef, you would be able to make object graphs accessible and only use the memory as required according to user actions.

[derrell]

If anyone is interested in my work-in-progress on reimplementing the Class and Property system, see the repository (and its README, with a list of what's already supported) at https://github.com/derrell/proxy-tests

Comments welcome. I'm making good progress.

[johnspackman]

That looks really good! 👍

Does this mean that we would not have any instances of qx.Class any more? Or perhaps they would just be a meta data that wraps the Proxy?

[derrell]

Thanks, @johnspackman. I don't understand your question, though. qx.Class is effectively a static class, or rather, it has only static members. I don't think there were ever any instances of qx.Class, were there?

[johnspackman]

you're right sorry - that was a not-enough-coffee moment :)

[johnspackman]

Properties and Async

There's been a few discussions about properties and how to handle async values in getters and setters; this comment is to clarify and discuss some ideas

Currently: getXxx always returns the actual value

getXxxAsync always returns a promise, via qx.Promise.resolve(this.getXxxx())

setXxxAsync is return qx.Promise.resolve(this.setXxx(value))

setXxx always checks the incoming value and if the check is not a Promise, it will look like this:

function setXxx(value) {
  function set(value) {
    var oldValue = this["$$user_xxx"];
    value = transformXxx(value); /* whatever transformation */
    /* ... do some check validations here ... */
    if (oldValue != value) {
      this["$user_xxx"] = value;
      var promise;
      promise = this.applyXxx(value, oldValue, "xxx");
      if (propertyDefinition.async) {
        promise = promise.then(() => fireEventAsync(/*...snip...*/));
        return promise;
      } else {
        // use `qx.event.Utils.track` to fire the event and track any promises in
        // a chain with the existing promise returned by `applyXxx`
        return a promise or the actual value from `qx.event.Utils.track`
      }
    }
  }
  var promise;
  if (qx.Promise.isPromise(value)) 
    promise = value.then(set.bind(this));
  else
    promise = set.apply(this, arguments);
}

What's happening in setX is that

(1) if the property value being set is a promise, it is resolved first

(2) if the property is async: true, then the setXxx method will always return a promise, but if it is async: false (the default) then it will try and return the actual value, unless the apply or fireEvent calls return a promise - in which case it will return a promise which resolves to the value being set.

await this.setXxx(qx.Promise.resolve(42));
this.assertTrue(this.getXxx() === 42); // ok

also

let promise = this.setXxx(qx.Promise.resolve(42));
this.assertTrue(qx.lang.Type.isPromise(promise)); // ok

[derrell]

I'm guessing that your setXxx() method intended to return promise; at the end.

We don't have that option. Now that we have an implementation that implements first-class properties, setXxx(value) is identical to xxx = value;. The expression value can't be changed in the process. setXxx() does not return a value to the user. We can return anything we want from setXxxAsync because that is always called as a method, not as a first-class operation.

[derrell]

In the new implementation, based on prior gitter discussion, when setXxx() is given a promise as the value, it looks to see if property.check == "Promise" and if not, it adds a .when() handler that replaces the value of the property in property storage with the resolved value of the promise. This means that depending on when you call getXxx(), you may get back the original promise that was saved when setXxx() was called (if it hasn't resolved yet), or you may get back the resolved value of that promise. This is the point of contention that started this discussion on gitter recently. The general consensus seems to be that once the promise is saved in property storage, it should not be changed in an unpredictable way; that it is the user's responsibility to await the promise if that is desired, not the property system's responsibility (in reference to synchronous properties).

[johnspackman]

I'm guessing that your setXxx() method intended to return promise; at the end. Yes

We don't have that option. Now that we have an implementation that implements first-class properties, setXxx(value) is identical to xxx = value;

That's fine - normally you do not want the return value and often discard it; if you need the promise, then you will have to use setXxx instead of the actual property, ie await this.setXxx(myPromiseValue)

In the new implementation, based on prior gitter discussion, when setXxx() is given a promise as the value, it looks to see if property.check == "Promise" and if not, it adds a .when()

Yes, I think that this is an improvement / bugfix. If the value is supposed to be a promise, then there is no reason to resolve it before setting the value because you already have the value

This means that depending on when you call getXxx(), you may get back the original promise that was saved when setXxx() was called

No, I think that would be wrong and its not how it is implemented now. It would break a lot of code. The getXxx returns the current value and this is the edge case where if the promise has not resolved yet then the value has not changed. A similar gap occurs with initialisation.

This is the point of contention that started this discussion on gitter recently.

Sorry if I've wandered around and caused some of the debate unnecessarily (it sounds like I did), but how it works now is how I think that it should work.

The general consensus seems to be that once the promise is saved in property storage, it should not be changed in an unpredictable way; that it is the user's responsibility to await the promise if that is desired, not the property system's responsibility (in reference to synchronous properties).

It's the setXxx that returns a promise if it has to, but the actual value if it does not. The change is that if the xxx property is supposed to store a promise (i.e. check: "Promise") then setXxx will store that value

The getXxx returns the value, even if the value has not changed yet.

Mutation

In the original proposal, I was suggesting detecting mutation and using promises to do it; this code was the example which would detect that .setXxx(somePromise) was still in progress:

if (myObject.isPropertyMutating(“xxx")) { 
  await myObject.resolveMutation(“xxx"); 
}

(although in hindsight, if we had an API to get to the property definition for an object instance, this might be better:

let promise = myObject.getProperty("xxx").promiseReady;
if (promise)
   await promise;

Note that even if you used myObject.xxx = someValue (where someValue is not a promise), and the apply method for xxx or an event handler for changeXxx returns a promise, then it is still mutating.

Something to consider here is that even if you said that .setXxx will not resolve a promise value, what about transform, apply, and event handlers? It is useful to allow them to return promises - which means to chain the promises, which means to resolve one after another - and the same applies to calling code. So even if you call .setXxx(42), you still might need to await the response because it's apply method or events may return promises.

[derrell]

The setX method executes the proxy set method which has its own return value (a boolean that means the value was saved; if it returns false, an error is thrown in strict mode). First-class properties, and asking for a return value from a setter, are mutually exclusive. I have ensured that calling a getter or setter via either its traditional method or via its first-class language construct are identical so that the two are entirely interchangeable.

I think optionally resolving a promise should be a feature of an async property, and not be involved at all in sync properties. Maybe that's a way forward. The other, and probably proper fix for this issue, since it seems to mostly involve bind, is to adjust the bind code to do the awaiting when awaiting is necessary. That code can ascertain whether it's working with a sync or async property for source or target, and do the necessary awaiting.

[derrell]

Mutation detection, although untested yet since merging the proxy-test implementation into qooxdoo, is implemented via the property method isAsyncSetActiveXxx(). It only applies to setXxxAsync(), however, not to synchronous setters.

[derrell]

In fact, this code that you presented above is implemented almost exactly like that, in setXxxAsync(). You just need to use setXxxAsync() and not setXxx() to get the desired behavior.

[johnspackman]

I think that there are two issues this is boiling down to: (a) the return value of setXxx (and not .xxx=value), and (b) whether .setXxx resolves the promise it is given.

I agree that if you need to await the result of the set, then you should await this.setXxxAsync, provided that there is always a .setXxxAsync (which there currently is only if async:true)

But making it so that the .setXxx only resolves the promise if the property is defined as async will cause problems unless you also want to rewrite binding as part of this proposal. Binding (IMHO) should be first class objects anyway so there is a value to a rewrite, but I've assumed that its non-trivial.

I think optionally resolving a promise should be a feature of an async property

What would you do if the apply method, or one of the event handlers from the change event, return a promise?

To me, this is all part of the same rabbit hole - setXxx can involve a promise, as can applyXxx(), as can fireEvent("changeXxx")

Mutation detection, although untested yet since merging the proxy-test implementation into qooxdoo, is implemented via the property method isAsyncSetActiveXxx(). It only applies to setXxxAsync(), however, not to synchronous setters.

That's a shame, because it's quite common to want to know if something is changing as part of a master "value" being changed - eg if the main value is in the process of being changed, the bindings for "value.abc" is also changing, but does not mean that the user is editing the widget that is bound to "value.abc"

(i.e. it is useful to know if a synchronous property is being changed)

[derrell]

I'd like @oetiker's comments too. I already agreed, somewhat reluctantly, to have the sync setter change the stored value of the property once the promise which was set in that property resolves (as long as property.check != "Promise"). I think it's ugly, but I'm able to deal with it. I'd be happier if it were explicitly requested via property configuration, e.g., resolvePromise : true.

The synchronous setter, setXxx() can not return a value. If you want to resolve the promise, then resolve it before calling the setter; don't expect the setter to resolve it for you, returning a promise.

The synchronous getter, getXxx(), always returns exactly the current value of the property. If that value is a promise, the promise is returned. And if a promise was set in the property and that promise has resolved, and we agree that resolved promises store the resolve value in the property automatically, then the resolved value is returned.

Other than that, my feeling is that traditional property features should be retained, as they are the most commonly used, and those are that property getters and setters are synchronous and do no magic other than when specifically requested, e.g., transform. I have no objection to adding intuitive and BC functionality, particularly as additional (non-traditional) methods such as get/setXxxAsync.

Where I started to get concerned was where it sounded like you wanted the return value from apply or an event handler to be able to modify the property's value. Maybe I misinterpreted. An apply method can reasonably set a new value in the property, but that's a separate operation that will (possibly) generate its own call to the apply method and fire events; it's not additional magic being added to the meaning of return values.

[johnspackman]

The synchronous setter, setXxx() can not return a value

Ok

getters and setters are synchronous and do no magic other than when specifically requested, e.g., transform

OK for transform

Where I started to get concerned was where it sounded like you wanted the return value from apply or an event handler to be able to modify the property's value.

No, definitely not. There's no way that would be predictable (how could you reapply a different value?) What an apply or event handler can do is to return a promise which needs to be awaited on in order to find that the top level set has completed.

[johnspackman]

WRT to the apply and event handler, the reason why a property (whether via a synchronous setter or async setter) would chain the promise is because you cannot have the event fire before the apply is finished; and events are sometimes used to communicate more than just one-off notifications (eg drag and drop).

Making setters work with promises all the time means that the calling code can respect the promises if it needs to, and ignore them if not.

[derrell]

A sync setter's apply is expected to be synchronous, traditionally. I may be able, however, for synchronous setXxx() calls, to obtain the return value of the apply method, and if it's a promise, delay the call to the event handler until after the promise resolves. That's BC because an apply methods' return value was previously undefined. That means, though, that contrary to traditional expected behavior, if the apply method returns a promise, the synchronous setter will return before apply completes, and therefore, also before the change event fires. I may be able to make that promise available to the caller via a promise getter, as is available for setXxxAsync() calls.

I do not believe this is the correct approach, though. I believe the correct thing to do is ensure that there is a setXxxAsync method on every property (not only on properties marked as async : true) and for user code (and bind or a new bindAsync) to call setXxxAsync instead of setXxx. That of course begs the question of whether the async : true flag even means anything any longer. Maybe it's not needed.

[derrell]

And what happens if apply returns a promise (recall that setXxx() returns before it resolves), and setXxx() is called again? This begins to incur the same chaining requirement as setXxxAsync, with more and more delay involved without any easy way to await it. The more I think about this, the more I think adding async functionality to the sync setter really over-complicates things, and we should re-think this.

[johnspackman]

A sync setter's apply is expected to be synchronous, traditionally.

Well, maybe but you could also say that was before async was supported at all.

If you force it to be synchronous (or ignore promises in the code for a synchronous setter) then you're saying that the behaviour of setting a property (ie side effects of apply/fireEvent) changes depending on how it was called; and as "calling" it can be done by bindings you're setting up a nightmare of bugs and unreadable stack trace that can be very tricky to track down.

I may be able, however, for synchronous setXxx() calls, to obtain the return value of the apply method, and if it's a promise, delay the call to the event handler until after the promise resolves.

That's how it works now.

There is a class called qx.event.Utils that facilitates all of this - it was tricky to write and it allows the setter (or event dispatch code) to return a promise if it, or anything further down the stack, needs to ... otherwise it resolves to an actual value.

The easiest way to understand it will be to look at the code that is generated for properties now - it's fairly simple and short, basically what I put in that snippet above.

Yes, that means that the return value is a literal or a promise that resolves to a literal, but that allows to avoid forcing the stack to change for non-promises (which is typical use) and avoid forcing everything to be async everywhere (which involves massive code refactoring)

That means, though, that contrary to traditional expected behavior, if the apply method returns a promise

IMHO it is useful to lean away from thinking about functions "returning a promise" - they are asynchronous, and this is getting baked into the language now. Returning a promise is just how we had to hack it until the language caught up (when we added Babel ES6->ES5 to the compiler)

For example this:

  async applyXxx(value, oldValue) {
    let blahDeBlah = await this.doSomething(value);
    /* return nothing */
  }

That means, though, that contrary to traditional expected behavior, if the apply method returns a promise, the synchronous setter will return before apply completes, and therefore, also before the change event fires.

Yes; generally speaking, this is actually acceptable in practice but it is something to be aware of. However, the apply method is "yours" ie in the class that the user is writing, so the user has control over it and can handle it. When the property change event is fired, if the apply method has not finished running, that is a disaster because whatever event handler there is will require that the apply has finished. So it is essential to chain the apply onto the fireEvent

If it is the caller who is providing a Promise, then the caller knows that there will be a delay and can await this.setXxx() (if they need to care about it)

It works out because the people who cause the async delay due to promises are in control of managing that async delay.

I may be able to make that promise available to the caller via a promise getter, as is available for setXxxAsync() calls.

That would be good, as would detecting mutation for synchronous calls.

I do not believe this is the correct approach, though. I believe the correct thing to do is ensure that there is a setXxxAsync method on every property (not only on properties marked as async : true) and for user code (and bind or a new bindAsync) to call setXxxAsync instead of setXxx. That of course begs the question of whether the async : true flag even means anything any longer. Maybe it's not needed.

I understand your concern, it's not perfect, any of it and part of the problem IMHO is that the language does not perfectly support async; the other part is that we want to retrofit this to existing code or change properties to be async during development.

Having to remember to use setXxxAsync variants though is a PITA and does not seem to achieve anything. If anything, the setXxxxAsync is (in hindsight) rather pointless and I very infrequently use it (can't think where I use it at all off the top of my head). (The getXxxAsync is a different story)

I dont think that we would benefit from a bindAsync either, because there is often a stack or chain of bindings and we'll never know which to use ... or we end up using bindAsync everywhere, in which case let's avoid s/bind/bindAsync/g for the sake of it

Again, we don't think of property "customer" as being asynchronous or synchronous, we think "oh, that's the customer object, it'll be mypkg.data.Customer class`.

Also, when we start using setXxxAsync, then it sounds like it could be that we would have to switch everything (to do with that class and uses of that class) to be Async variants.

And what happens if apply returns a promise (recall that setXxx() returns before it resolves), and setXxx() is called again? This begins to incur the same chaining requirement as setXxxAsync, with more and more delay involved without any easy way to await it.

That's what I meant that promises do not absolve you of the issues surrounding threading, just a less violent crash.

You have the delay anywhere that a promise is created, although it is actually very efficient AIUI. However the qx.event.Util class takes care of a lot of this, and only causes the delay if absolutely necessary

However, this way we have the opportunity to trap those kinds of issues with a runtime warning and give the user what they expect which is "just set this value X to property Y". Given that arguably modern use of functions do not return promises (except under the hood), the whole "is X a promise or not" is overhead for the user.

It is also the case (in my experience anyway) that 99% of the time the framework is taking care of all these async issues and the user does not care. When you do, you can take control with manually binding and await ... in just the spot that needs it.

The more I think about this, the more I think adding async functionality to the sync setter really over-complicates things, and we should re-think this.

We can rethink it, but I am absolutely clear that it is necessary and workable. I have lots and lots of code that works very well on these principals, huge object graphs which load themselves (ie populate property values) on demand, and was retrofitted into a largely synchronous code base when I added the Promise support.

My experience is as I've said, sometimes you have to await the promise at the top of the stack, mostly you don't care so long as things happen in the order that they were intended.

[derrell]

I am trying to retrofit promise support into the sync setter. This method no longer works propertly for normal (non-property) set, now that I've mucked with it. I don't understand what is supposed to happen. This is based on the setXxxAsync code. It seems that this[activePromiseProp] is never reset to null, which I think requires something to do with tracker, but isn't correctly implemented...

      /**
       * Call the `apply` method and fire an event, if required. This
       * method is called by both `setX()` and by `initX()`
       *
       * @param proxy {Proxy}
       *   The object on which the property resides
       *
       * @param property {Object}
       *   The property configuration
       *
       * @param propName {String}
       *   The name of the property described by the configuration in
       *   `property`
       *
       * @param value {Any}
       *   The new property value
       *
       * @param old {Any}
       *   The current (old) property value
       *
       * @param variant {"init"|"init->set"|"set"|null}
       *   The internal variant at the time the property is being
       *   manipulated,which affects whether the `apply` method is called and
       *   the event fired.
       */
      __applyAndFireEvent(
        proxy,
        property,
        propName,
        value,
        old,
        variant)
      {
        let  activePromise;
        let  tracker = {};
        let  propertyFirstUp = qx.Bootstrap.firstUp(propName);
        let  activePromiseProp = `$$activePromise${propertyFirstUp}`;

        // Ensure we're dealing with a synchronous property, and
        // either the old and new values differ, or we're in one of
        // the state variants where we need to call the `apply` method
        // and generate a change event even if old and new values are
        // the same. (Async properties' apply method is called
        // directly from `setPropertyNameAsync()`, not from here )
        if (property.async ||
            (property.isEqual.call(proxy, value, old) &&
             ! ["init", "init->set"].includes(variant)))
        {
          return;
        }

        let             applyAndFireImpl =
          () =>
          {
            // Is there an apply method?
            if (property.apply)
            {
              // Yes. Call it.
              if (typeof property.apply == "function")
              {
                property.apply.call(proxy, value, old, propName);
              }
              else // otherwise it's a string
              {
                proxy[property.apply].call(proxy, value, old, propName);
              }
            }

            // Are we requested to generate an event?
            if (property.event)
            {
              const Reg = qx.event.Registration;

              // Yes. Generate a sync event if needed
              if (Reg.hasListener(proxy, property.event))
              {
                qx.event.Utils.track(
                  tracker,
                  Reg.fireEvent(
                    proxy,
                    property.event,
                    qx.event.type.Data,
                    [ value, old ]));
              }

              // Also generate an async event, if needed
              if (Reg.hasListener(proxy, property.event + "Async"))
              {
                qx.event.Utils.track(
                  tracker,
                  Reg.fireEvent(
                    proxy,
                    property.event + "Async",
                    qx.event.type.Data,
                    [ qx.Promise.resolve(value), old ]));
              }
            }

            // What is this doing???? Nothing, I think...
            if (tracker.promise)
            {
              tracker.promise.then(() => value);
            }
            // ^^^^^^^^^^^^^^^^^^^^^^

            // If we are the last promise, dispose of the promise
            if (activePromise === this[activePromiseProp])
            {
              this[activePromiseProp] = null;
            }

            return qx.Promise.resolve(value);
          };

          // If this property already has an active promise...
          if (this[activePromiseProp])
          {
            // ... then add this request to the promise chain
            activePromise =
              this[activePromiseProp].then(applyAndFireImpl);
          }
          else
          {
            // There are no pending requests. Begin this one right now.
            activePromise = applyAndFireImpl();
          }

          // Save the new head of the active promise chain
          this[activePromiseProp] = activePromise;
      },

[johnspackman]

In this bit:

      // What is this doing???? Nothing, I think...
      if (tracker.promise) {
        tracker.promise.then(() => value);
      }
      // ^^^^^^^^^^^^^^^^^^^^^^

What it is doing is ignoring the accumulated return value from the promise and replacing it with the value being set; this is because abc.setXxx(42) === 42 but the promises returned by apply or events can return other values, trapped in the promise; if you just return the promise as is, then it returns whatever the apply/events returned.

I'm working on a new version, back in a sec

[derrell]

Thanks. I believe the referenced code in that snippet doesn't do anything because the return value of .then is ignored... but there's nothing to do with it, because __applyAndFireEvent method does not return a value (although applyAndFireImpl does).

[johnspackman]

I havn't tested this, but I've had a good go at getting it there; the key thing is that you never explicitly create promises, you relay on apply/fireEvent to do that for you ... but you use qx.event.Utils.track to simulate the .then(...) part, and it will decide if it needs to use a promise or if it can execute the code synchronously.

The "is it complete" promise is created out of line and then resolved by the last setter invocation - but if noone ever asked if the property set is complete, no promise is created.

  /**
   * Returns a promise that will be resolved when the property has finished being set, or null
   * if the property is not being set.  Only creates a promise on demand
   *
   * @param proxy {Proxy}
   *   The object on which the property resides
   *
   * @param property {Object}
   *   The property configuration
   *
   * @param propName {String}
   *   The name of the property described by the configuration in
   *   `property`
   *
   * @returns {qx.Promise}
   */
  getPropertySetCompletePromise(proxy, property, propName) {
    let propertyFirstUp = qx.Bootstrap.firstUp(propName);
    let setInProgressName = `$$setInProgress${propertyFirstUp}`;

    // Do we already have a promise that will be completed?
    if (proxy[setInProgressName]?.onSetCompletePromise) {
      return proxy[setInProgressName].onSetCompletePromise;
    }

    // Are we setting a value?  Then we need a promise to be completed
    if (proxy[setInProgressName]?.refCount) {
      return (proxy[setInProgressName].onSetCompletePromise = new qx.Promise());
    }

    // Not being set
    return null;
  },

  /**
   * Call the `apply` method and fire an event, if required. This
   * method is called by both `setX()` and by `initX()`
   *
   * @param proxy {Proxy}
   *   The object on which the property resides
   *
   * @param property {Object}
   *   The property configuration
   *
   * @param propName {String}
   *   The name of the property described by the configuration in
   *   `property`
   *
   * @param value {Any}
   *   The new property value
   *
   * @param old {Any}
   *   The current (old) property value
   *
   * @param variant {"init"|"init->set"|"set"|null}
   *   The internal variant at the time the property is being
   *   manipulated,which affects whether the `apply` method is called and
   *   the event fired.
   */
  __applyAndFireEvent(proxy, property, propName, value, old, variant) {
    let propertyFirstUp = qx.Bootstrap.firstUp(propName);
    let setInProgressName = `$$setInProgress${propertyFirstUp}`;

    // Ensure either the old and new values differ, or we're in one of
    // the state variants where we need to call the `apply` method
    // and generate a change event even if old and new values are
    // the same.
    if (property.isEqual.call(proxy, value, old) && !["init", "init->set"].includes(variant)) {
      return;
    }

    if (!proxy[setInProgressName]) {
      proxy[setInProgressName] = {
        refCount: 1
      };
    } else {
      proxy[setInProgressName].refCount++;
    }

    let promise;

    // Is there an apply method?
    if (property.apply) {
      // Yes. Call it.
      if (typeof property.apply == "function") {
        promise = property.apply.call(proxy, value, old, propName);
      } // otherwise it's a string
      else {
        promise = proxy[property.apply].call(proxy, value, old, propName);
      }
    }

    let tracker = {};
    if (promise) {
      qx.event.Utils.track(tracker, promise);
    }

    var Reg = qx.event.Registration;

    // Yes. Generate an event if needed
    if (Reg.hasListener(proxy, property.event)) {
      qx.event.Utils.track(tracker, () => Reg.fireEvent(proxy, property.event, qx.event.type.Data, [value, old]));
    }

    // Also generate an async event, if needed
    if (Reg.hasListener(proxy, property.event + "Async")) {
      qx.event.Utils.then(tracker, () =>
        Reg.fireEvent(proxy, property.event + "Async", qx.event.type.Data, [qx.Promise.resolve(value), old])
      );
    }

    qx.event.Utils.then(tracker, () => {
      proxy[setInProgressName].refCount--;
      if (proxy[setInProgressName].refCount == 0) {
        let promise = proxy[setInProgressName].onSetCompletePromise;
        proxy[setInProgressName] = null;
        if (promise) return promise.resolve();
      }
    });

    if (tracker.promise) {
      return tracker.promise.then(() => value);
    }
    return value;
  }

This function above is suitable for setXxx and setXxxAsync; the difference is that setXxxAsync always returns a promise, so you would use qx.Promise.resolve:

  setXxx = value => this.__applyAndFireEvent(proxy, property, propName, value, old, variant);

  setXxxAsync = value => qx.Promise.resolve(this.__applyAndFireEvent(proxy, property, propName, value, old, variant));

Also the code was referring to this[$$activePromise..., shouldn't that be proxy[$$... ?

[johnspackman]

PS I changed a few names to make more sense to me as I was writing it, feel free to change them/back etc

[derrell]

Let me attempt to absorb this. Yes, this should have been proxy (copy/paste and lack of understanding causing that error).

For setXxxAsync, the implementation uses await and (I think) works as desired. I wouldn't change that if I didn't have to, as that code is easier to understand and follow.

[derrell]

getPropertySetCompletePromise is never called. When is it supposed to be used?

[johnspackman]

getPropertySetCompletePromise is the implementation of whatever function you wanted to make available so that the user can find out if a given property is currently being set, either synchronously or asynchronously.

[derrell]

__applyAndFireEvent does not return a value since it is used only for the synchronous setter. Therefore the lines at the end don't serve any purpose in this implementation, I think:

    if (tracker.promise) {
      return tracker.promise.then(() => value);
    }
    return value;

[johnspackman]

Personally I would just have the one implementation. The other version can't be that much simpler than the one above can it?

I see no harm in setXxx returning a promise or a value, because as I said setXxxAsync is pretty pointless in practice ... unless you force users to use the setXxxAsync version just because they want to await it, which sounds like creating a rationale.

What your proposing is a non-BC change and I'm struggling to see the justification of changing it, especially as the function I sketched out above seems to make the whole thing pretty clear.

The more I think about it, I want setXxx to maintain its current behaviour- I haven't checked but I'm concerned that I will be impacted in lots of hard-to-find ways, where I will simply be replacing setXxx with setXxxAsync for no benefit.

They are effectively exactly the same, the only difference is that setXxxAsync always returns a promise.

The only really useful thing will be getXxxAsync - I say "will" because the property system does not have on demand properties yet.

[derrell]

At the expense of being repetitive, the setter _can not__ return a value without a hard-to-implement and hard-to-get-right kludge. o.setX(value) executes the proxy set method. It is exactly equivalent to o.x = value;. One of my primary motivations for doing this work was first-class properties. The setter, o.setX(value), does not return a value.

[johnspackman]

I guess I'll have to see your code to understand the problem, but I would have thought that it's just an internal api call.

I think it's reasonable to expect BC, and I'm not convinced that an implementation detail is justification for breaking an established api.

[derrell]

No, it's not an internal api call. It's a trap into the proxy handler, which doesn't return a value to the "caller". It could be kludged, but I'm not convinced (at all) that it's the right thing to do.

Can you point me at the documentation that discusses returning promises from the sync setter? I've never seen it, and until working on this project, had no idea it was being done. I don't feel that there should be a return value from a sync setter, and this is the correct time to change things if there is a better design for something that is not widely in use yet.

[johnspackman]

We've had quite an extensive set of conversations as I've tried to explain how and why it works the way the it does - there's a lot of issues to get to navigate, and like a lot of things in the framework there is functionality which is baked into the code which is documented at a high level only - events for example. That's not a justification to break things historically.

This functionality has been in place for 6 years (since January 2017).

To write the code for you yesterday, I simply picked a random property which has an apply and event, got the source at runtime, and pretty printed it. I then refined it a bit to take advantage of the fact that it no longer has to go through a code-generation process.

I don't feel that there should be a return value from a sync setter, and this is the correct time to change things if there is a better design for something that is not widely in use yet.

I feel that I've explained several times why it works the way it does, what the benefits are, and what the disadvantages of forcing users to switch to setXxxAsync are - I'm talking from a position of quite a lot of practical experience here, this is not theory.

Please can you point me at the Proxy code so that I can understand exactly what the issue is?

[derrell]

I think I came up with a reasonable compromise that will retain o.setX(value) identical to o.x = value yet still provide the capability you're looking for. This is yet untested, but I think it should work. The block of code that you suggested would return either the promise or the value:

    if (tracker.promise)
    {
      return tracker.promise.then(() => value);
    }
    return value;

has been changed to

          if (tracker.promise)
          {
            proxy[syncSetResultProp] = tracker.promise.then(() => value);
            return;
          }

          proxy[syncSetResultProp] = value;

and method getLastSyncResultX() added. This lets you do:

o.x = 23; // or o.setX(23);
setResult = o.getLastSyncResultX();

This retains the cleanness of the API (setter does not return a value), but provides you with the information you're looking for.

I'm trying to be accommodating here, as you can see, but I think I'm not the only one who believes that the sync setter should not return a value (or, for that matter, that promises should be resolved in a sync setter at all, changing the property value unpredictably). My arguments are for technical solutions that make sense going forward, at the possible expense of a very small number of users BC (especially when the BC breaks are undocumented and mostly unknown features). Hopefully you find the above an acceptable solution to this issue.

[johnspackman]

I appreciate that o.x = 23 cannot return anything, because that is baked into the language but I'm not talking about that.

The code o.setX(23) should act like it always has, which is to return the value that it was given. If you use await o.setX(qx.Promise.resolve(23)) the same thing happens, in that it returns a promise which resolves to 23, but it kind of has to....

What you're proposing is that now needs to be changed to await o.setXAsync(qx.Promise.resolve(23)). It seems that the only justification for this change is that there will be a change in the code that you find kludgy, but I don't understand why it's so hard because you've also implemented setXAsync so I don't see why you can't implement setX (especially as the code I gave you will handle both versions).

Given the net effect of the change for the user is zero, what is the point except to insist that async usage has the word "Async" tacked on the end, something which is not required anywhere else?

Probably the easiest way for me to understand will be to read the code? Do you have it committed so that I can take a look?

[derrell]

Please can you point me at the Proxy code so that I can understand exactly what the issue is?

The issue is not just in the code; it's a technical decision issue. That said, the code is in the qooxdoo github repo in branch new-class-property-system

[johnspackman]

The issue is not just in the code; it's a technical decision issue

Please can you reiterate the technical reason why it should be the case that .setX() (not the o.x = 23 setter) should be changed to not return a promise, if a promise was involved in processing the setter?

[derrell]

What you're proposing is that now needs to be changed to await o.setXAsync(qx.Promise.resolve(23)). I think you misunderstood my compromise.

No, you don't need to switch to o.setXAsync() if you were previously using o.setX(). What I've done is allowed you to continue to use o.setX() (although I still feel that's wrong; the sync setter shouldn't be processing promises in any way, shape, or form). The only difference is that you don't await the return value from o.setX(); you instead await the return value of o.getLastSyncSetX().

It seems that the only justification for this change is that there will be a change in the code that you find kludgy, but I don't understand why it's so hard because you've also implemented setXAsync so I don't see why you can't implement setX (especially as the code I gave you will handle both versions).

Please can you reiterate the technical reason why it should be the case that .setX() (not the o.x = 23 setter) should be changed to not return a promise, if a promise was involved in processing the setter?

I think I did so in the above paragraphs, but maybe I can be more clear. Firstly, I believe that o.setX(value) and o.x = value should continue to be identical. There's even a big comment block in the code that handles the setX method:

                  // *** CAUTION FOR DEVELOPERS!!! ***
                  // Do not put any code other than `this[prop] =
                  // value` into the `set` property method here. This
                  // code is called only via `o.setX(value)` and not
                  // via `o.x = value`. To ensure the reliability of
                  // first-class properties, i.e., that those two
                  // statements work identically, all code for setting
                  // must be in the proxy `set` handler, not here.
                  // *** CAUTION FOR DEVELOPERS!!! ***

Secondly, I don't feel that the sync setter, setX(), should be doing anything with processing promises, and returning a promise from it means endorsing processing of promises in the setter, which I do not.

Does that help explain my technical argument?

[johnspackman]

Does that help explain my technical argument?

No, you've mostly said that it should be your way because you believe that it should, and mentioned a big, 3-day old, comment.

AFAICT the only technical problem is that you've implemented all the set code within the setter for x, and that means that you can only trigger that code by setting the native value, ie by writing o.x=23 (or this[propName] = value) and that means that when you implement setX and setXAsync you are forced to follow the JS for native property access.

IE you have this:

handler = {
  set  : function(obj, prop, value) {
    /** all major code goes in here */
  }
};
/* ... snip... */
let propertyMethodFactory = {
  set : function(prop, property) {
    this[prop] = value;
  }
};

where you could have:

__theSetMethodForProperties(obj, prop, value) {
    /** all major code goes in here */
},

/* ... snip... */

  handler = {
    set(obj, prop, value) {
      this.__theSetMethodForProperties(obj, prop, value);
    }
  };
  let propertyMethodFactory = {
    set(prop, property) => this.__theSetMethodForProperties(obj, prop, value)
  };

There is no harm in refactoring the set code a separate function with it's own API that is used by both the native setter in the Proxy and in the implementations of .setX() and .setXAsync()

Secondly, I don't feel that the sync setter, setX(), should be doing anything with processing promises

We've already agreed that binding would be broken without it, which would make it chronically difficult to add async to an application.

, and returning a promise from it means endorsing processing of promises in the setter, which I do not.

I completely get the attraction of being clear about "this is an async call" but in practicality it is just not necessary (or helpful) to make that distinction.

It is quite possible that i am being over cautious about the return value of setX, and I'll have to actually try your branch without it to find out, but I don't think you've advancing any justification for changing 6 year old code other than you feel it would be bad.

We all have feelings based on our experience of how something should be, and Im not knocking that, but at the same time, I'm not looking forward to a huge testing program just to find out if making this change will break my apps. And it is definitely not fair to say it's OK to do this now because maybe i'm just one of a very few (or perhaps the only one) who will be affected.

Over the past 6 years since I implemented promises, I've steadily moved a huge code base from synchronous (with a smattering of callbacks) to a lot of promise based code, and 9 times out of 10 I simply do not have to care whether the incoming value is a value or will resolve to a value. Not once do I wish that I'd added the word "Async" to the end of something. In fact, I have loved the way that (largely thanks to bindings), async has been an easy migration.

[cboulanger]

My opinion is: we should not break established behavior unless it is absolutely necessary. As far as I have understood the issue, there is no harm in returning promises from setXXX(), there is no technical necessity for changing the behavior, since setXXX has become a mere BC wrapper for native properties support. Databinding via proxy needs to be re-implemented completely anyways, I think, therefore the old system (including async property support) should continue as before.

[oetiker]

There has been written so much that I have a bit of a problem following along ... I gathered there are two issues now.

a) should setX(x) return x or nothing. b) should, given setX(Promise.resolve(44)), the call getX() return a Promise resolving to 44 or should it return 4 directly.

I also gather, that @johnspackman has a big interest in qx8 NOT breaking all his existing code which uses the current behavior Re promises.

I guess we should find a solution the does:

a) not break Johns code (or any other that has been created) b) behaves the same way as 'normal' javascript regarding properties. Maybe this is already the case, but I did not follow the arguments in this respect.

[derrell]

Since others believe that it's important that we be backward compatible with this piece of code, I will arrange to do so. There is nothing that documents this feature, AFAIK, and there are no unit tests that I can find that look at the return value of the setter, or await it. I don't know what to search for; maybe there are. If so, please point me to them.

It does seem a bit disingenuous to be gung ho on eliminating the initX() method entirely ("my vote is that we take advantage of @derrell 's rewriting to clean this mess up and take the hit of some BC changes" from https://github.com/qooxdoo/qooxdoo/discussions/10449#discussioncomment-3436722), which would affect a great many apps and users, but be highly resistant to a change that would affect many fewer users.

Anyway, please provide documentation for this feature, and unit tests, and I'll see what I can do about implementing it.

[johnspackman]

It does seem a bit disingenuous to be gung ho on eliminating the initX() method entirely

I guess I was rushing to work through some ideas, and in fairness I think it seemed easy to implement a change (by hand editing or by compiler like es6ify) to eliminate initX, especially as it all seemed a bit of a mess until you looked into the code to find out what exactly was going on. As it turned out, eliminating initX is not as simple as it seems (or even a good idea) and it had to stay.

Anyway, please provide documentation for this feature, and unit tests, and I'll see what I can do about implementing it.

OK, although the code I wrote yesterday is a pretty detailed example

[derrell]

Ah, no, I don't mean documentation on how to implement it; I mean documentation for the manual, that documents its expected behavior and how and when to use it.

[derrell]

But of first importance, since I can't do anything with the implementation until they're done, are the unit tests that show it working properly.

[derrell]

@johnspackman Is this code correct? As I step through it, in a test where the apply method returns a promise which has not yet resolved, I'm finding that Reg.fireEvent is being called anyway, so it's getting to the event listener before the promise returned by the apply method resolves. I don't believe that's what's supposed to happen, right?

        // Is there an apply method?
        if (property.apply)
        {
          // Yes. Call it.
          if (typeof property.apply == "function")
          {
            promise = property.apply.call(proxy, value, old, propName);
          }
          else // otherwise it's a string
          {
            promise = proxy[property.apply].call(proxy, value, old, propName);
          }
        }

        if (promise)
        {
          qx.event.Utils.track(tracker, promise);
        }

        // Are we requested to generate an event?
        if (property.event)
        {
          const Reg = qx.event.Registration;

          // Yes. Generate a sync event if needed
          if (Reg.hasListener(proxy, property.event))
          {
            qx.event.Utils.track(
              tracker,
              Reg.fireEvent(
                proxy,
                property.event,
                qx.event.type.Data,
                [ value, old ]));
          }

[derrell]

I have confirmed that promise (set via the return value of the apply method) does appear to be a qx.Promise, and while stepping, see that the qx.event.Utils.track(tracker, promise); code is called.

[johnspackman]

As I said at the time, I didn’t test it just wrote it as best I could from the generated code.

If you like I’ll have a go on your branch, but not sure when I’ll be able to do it.

If the apply finishes with a promise before the event is fired that would be a bug because obviously event handlers are entitled to expect that getValue will return the correct value

Sent from my iPhone

On 2 Sep 2022, at 18:33, Derrell Lipman @.***> wrote:

 I have confirmed that promise (set via the return value of the apply method) does appear to be a qx.Promise, and while stepping, see that the qx.event.Utils.track(tracker, promise); code is called.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[johnspackman]

Unit test added https://github.com/qooxdoo/qooxdoo/pull/10455

Documentation already exists at https://github.com/qooxdoo/qooxdoo/blob/master/docs/core/promises.md#creating-asynchronous-properties

The unit test shows that I was wrong about how setXxx works - although the internal method returns a promise or a value, the setXxx code which is generated and which uses that method always returns the value that was passed in, even if this means that a promise is still waiting to be resolved.

The setXxxAsync method works as described above

As far as I can tell, everything works correctly and as expected (and matches the documentation) - I am disappointed that setXxx does not return a promise, but this will be because the setXxx methods have always returned that value and it had to be this way for BC.