DerivedProperty should support TEmitter to update its value

[zepumph]

I have been hitting this on and off for years, and it sounds like @jonathanolson and @samreid do too.

[zepumph]

The proposal: A new option to DerivedProperty which will be a list of emitters, in which whenever they fire, the DerivedProperty calls recomputeDerivation().

I think my favorite name is emitterDependencies. I'll ask devs for input.

[zepumph]

It would be kinda cool if the emitters could be inlined into the same dependency list, but that sounds like a nightmare for TypeScript support, and I would rather not even try to do that.

[zepumph]

On slack @pixelzoom had some thoughts about this proposal. I'll assign to him for some comment.

[samreid]

We eventually found a better way, but we temporarily added this workaround to simulate an Emitter and trigger a DerivedProperty update when desired:

https://github.com/phetsims/projectile-data-lab/commit/5cb53efbc9d3f018a4415652c194bb8d33779818

We saw a case in https://github.com/phetsims/sun/blob/1f2cdadcfb30bda951a5e633a607fe04437f88c7/js/Carousel.ts#L237-L243 that matches this pattern.

[zepumph]

Michael Kauzmann 4 hours ago:

There is a proposal in https://github.com/phetsims/axon/issues/451 to add a new option to DerivedProperties so that you can pass through Emitters to it as well. This way your derived Property knows to update when those emitters fire in addition to any dependency Properties changing. Please note in that issue if you have opinions on the option name, currently I'm thinking emitterDependencies. This is just for DerivedProperty :+1: 1

9 replies

Chris Malley 3 hours ago:

I’m out today, so can’t get to the axon issue right now. But this sounds like an undesirable complication of DerivedProoerty, which is already too complicated. And when an Emiter does fire, how do you propose to pass the emit args to the derivation callbacks?

Michael Kauzmann 3 hours ago:

I do not propose that we do that.

Michael Kauzmann 3 hours ago:

Happy to hold off. I wasn't planning to work on it today. I just wanted to start the conversation. I'll assign to you for comment. No timeline or anything, and it isn't blocking anything. (edited)

Chris Malley 3 hours ago:

I’d like see a use case that requires this, to understand why one can’t simply factor out a function that is used by both a DerivedProoerty callback and Emitter callback.

Chris Malley 3 hours ago:

If you’re not going to somehow provide emit args to the derivation, then you don’t have a real solution for Emitter.

Michael Kauzmann 3 hours ago:

Right. That makes sense.

Sam Reid 3 hours ago:

Would it make sense to only support Emitter[0]? Emitter args are transient and we wouldn’t want to cache them.

Sam Reid 3 hours ago:

(otherwise they would be Properties)

Chris Malley 45 minutes ago:

If you were to support only Emitter[0] and enforce that, then that makes the emit args problem moot. If emitterDependencies contains multiple Emitters, you’d also better not care which Emitter fired. Also questionable about why a DerivedProperty callback should return a different value due to an Emitter firing — derivation is supposed to be dependent on dependency values. If you are not able to enforce only Emitter[0], and allow any Emitter, then I suspect that we’ll see creative ways of reaching into whatever called emit to get the arg values in a different way — spaghetti code. So to me, this all seems like a lot of work and complication in DerivedProperty for dubious gain. Again, what’s the use case that can’t be solved by factoring out a common function that uses both as the callback for both a DerivedProperty and Emitter?

[zepumph]

I believe it is a reasonable decision to allow for the firing of Emitters to trigger a DerivedProperty to recompute. I also understand how that would only being adding half-support for Emitters, which is less than ideal. It would be good to get a bit more information from devs about how people have wished to use this.

I don't believe we need to decide to fully support Emitters in order to gain value from adding a listener to them to trigger recomputation. It is just a cost/benefit analysis. By adding it as an option, it feels less invasive, but doing so would not provide much of a path to providing emit args to the derivation function in the future. I think a sync discussion will be the most efficient and prudent path froward from here, and I'm going to add it to the dev meeting agenda to get everyone's opinion about the benefit of something like this.

[pixelzoom]

My feedback was captured in https://github.com/phetsims/axon/issues/451#issuecomment-2035398932.

I still have not heard the answer to:

Again, what’s the use case that can’t be solved by factoring out a common function that's used as the callback for both a DerivedProperty and Emitter?

[zepumph]

@samreid and I discussed, and we agree with @pixelzoom that DerivedProperty is too complicated for this feature. We could see this being a nice subtype, but we also saw logic for deferring that may complicated that path.

Only one usage of recomputeDerivation is this exact case. Unfortunately I feel like there may be others, but they would be hard to find generally.

@samreid also had an idea for an EmitterProperty, that wraps an Emitter and keeps a value. That way we could just provide that to the DerivedProperty. Mostly all solutions we have thought of seem more complicated than their value.

We will sit on this over the weekend and see if we should just close this issue.

[zepumph]

@samreid and I feel that this issue can be closed. @pixelzoom's suggestion along with the doc added above more than take care of any cases that may arise. If new need comes up, then we can handle that then.