Famous / engine

MIT License
1.75k stars 250 forks source link

feature request to add callback for DOMElement.onShow #463

Open sonwh98 opened 9 years ago

sonwh98 commented 9 years ago

I want to mount a react component on a DOMElement when its visible. DOMElement.onShow doesn't take a call back . It would be useful if it did. Can this be a feature request?

sonwh98 commented 9 years ago

There are a bunch of "on" methods in DOMElement.js Isn't it idiomatic for those lifecyle events to take callbacks so that developers can write code to participate in the lifecycle of the component?

sonwh98 commented 9 years ago

upon closer inspection it seems these "on" methods are not lifecycle methods. Very confusing because I am use to seeing "on" methods used as lifecycle methods that allow me to put call backs to participate in the lifecycle events. It seems the only way to participate in the lifecycle is through inheritance.

michaelobriena commented 9 years ago

Yea, sorry for the confusion. The onMethods get called by the nodes when the event occurs. They are not registrations.

michaelobriena commented 9 years ago

didShow may be a better name

sonwh98 commented 9 years ago

thanks @michaelobriena . is there an easy way to have a call back function triggered when the onShow method is called?

alexanderGugel commented 9 years ago

@sonwh98 Components enable event handling. The DOMElement's on... methods are used for receiving the events, not for registering handlers. In order to call a function whenever a node is being shown, a new component needs to be added to the Node:

var node = new Node()
node.addComponent({
  onShow: function () {
    // your code goes here
  }
})
sonwh98 commented 9 years ago

@alexanderGuge thanks for answering. These "on" functions are inconsistent. For example, the onMount gets passed in the parent node and the component's id (aka index), but the onShow does not. Is there a reason for this? the onShow should also be passed in the parent node

I'm writing a clojurescript DSL to allow constructing the scene graph declaratively using clojure datastructure. For example:

{:node/id "foobar"
 :node/mount-point [0.5 0.5 0.5] 
 :node/components [ {:onMount (fn [node index] 
                                    (println "onMount node=" node " index=" index]))}
                    {:onShow (fn [] 
                                    (println "onShow called. mount react component"))}]}

It would be nice if the onShow had the same function signature as the onMount so that I can write code to attach a react component onto the famous component's containing Node.

In your example, the node is in the component's lexical scope, but in ClojureScript DSL it is not. The node is constructed later so it needs to be passed in like the onMount

sonwh98 commented 9 years ago

@alexanderGuge I can refactor the onShow and submit a pull request or maintain my own fork (which I prefer not to do). Before I do that, is there a reason for onShow having a different parameter list than onMount?

alexanderGugel commented 9 years ago

The function signatures are different because the events are different. E.g. onSizeModeChange receives the changed size modes, onTransformChange receives the changed transform, onOpacityChange receives the changed opacity, onAddUIEvent receives the name of the added UI event.

This is not a inconsistency, but a design decision we made. If you decide to ignore the arguments altogether, you can still access them directly via the Node.

We do not intend to change the function signatures, although I agree that it would be "nice" to always pass in the node as a first argument. This would be a breaking change. It is unlikely that it would be merged.

Discussions about changing the way on functions behave is out of scope of this issue IMO.

To address the original issue: The on functions are a way to receive events, they are a replacement for callbacks in this case. Does this answer your original question?

sonwh98 commented 9 years ago

I should have phrased the question differently. I was suggesting that the "on" methods of components be passed the node as the first parameter. Otherwise, if a component needs access to the node in the onShow method, it will have to implement an onMount method and set an internal state variable to the node so that its accessible in the onShow. I'm implementing components from the clojurescript side and without access to the node in the "on" methods, I will have to do what I just describe above and muck around with mutating the "this" pointer. From a Clojure programmer's perspective, this feels dirty.

Even from a Javascript programmer's perspective, having the node passed in as the first parameter of those "on" methods makes things easier. For example, if I needed to do something with the node in the onShow, I would have to implement an onMount that does nothing more than set a node variable on the "this" pointer. This feels like a waste of brain power because someone has to write and implement extra code just because onShow does not have the node passed in. I think you agree that this is good but would require too much work right?

alexanderGugel commented 9 years ago

This would literally take 5 minutes to change, but it would break every application out there that is using those methods. While I'm a friend of breaking things for the sake of a better API, the proposed change would lead to tons of subtle bugs in dependent code. I'm not sure if we want this.

I agree that the function signature should have been like you proposed from the beginning on.

@michaelobriena Thoughts?

sonwh98 commented 9 years ago

famous hasn't reached 1.0 yet so breaking backwards compatibility for a cleaner API is worth it IMHO. Its a small change not a complete rewrite like what was done a few times already lol... so there is already precedent for breaking backwards compatibility for the better :)