feat: Add core/Component.js #211
Conversation
|
I would prefer a component factory opposed to inheriting personally. Anyone else care to weigh in? |
|
I think that we should have a component class to inherit from. This doesn't preclude having a component factory as part of our api, which I agree we should have, but does improve code quality in any case. That being said, there are other things in this pr. |
| if (item && item.onUpdate) { | ||
| item.onUpdate(time); | ||
|
|
||
| if (item.onUpdated) item.onUpdated(time); |
There was a problem hiding this comment.
What is, onUpdated? Given that it is called two lines after onUpdate with the same time argument, what is its purpose?
There was a problem hiding this comment.
onUpdated is being called on the component after the onUpdate method of the component has been called without requiring the component to have an update method.
The components _requestUpdate method sets this._requestingUpdate to true. After the component has been updated, we need to clear this flag again. This would need to happen after the component has been updated. This would mean that the component would have to have an onUpdate method. I wanted to avoid the need to call call the super classes onUpdate method when subclassing the component.
There was a problem hiding this comment.
Not sure if that's better at this point. Especially considering the fact that the API is now kind of inconsistent - e.g. there is no onPreUpdate. Just gonna remove that.
|
@DnMllr Resolved. Anything else? |
|
@michaelobriena Rebased. |
alexanderGugel
force-pushed
the
Component
branch
3 times, most recently
from
92e6d9d to
620417b
Compare
|
Can this be merged? Why has this been labeled as controversial? |
|
@alexanderGugel we can't remove onMount from adding a component. You're probably correct that onAdd and onRemove should be different methods for than onMount onDismount, but if component state is determined by their event driven interface then a component that is added to a mounted node is itself becoming mounted and so will need to hear the onMount event. Same for onDismount. |
|
@DnMllr Components can still react to onMount and onDismount. Distinguishing between adding components and actually mounting the node doesn't change the fact that onMount is still an event. Calling onMount on a component when the component is being mounted is conceptually incorrect, since it results into the fact that there is no way to know for the component when it actually has been added as opposed to when the node has been mounted. I feel pretty strongly about this, but I'm well aware of the fact that onAdd is not part of the nodes "event driven interface". I strongly disagree with what we currently have, since it makes it impossible for the component to know when its node has actually been mounted. When the component's onMount method is being called, it can not (easily) know whether the node has "just" been mounted, or if it was mounted before the component has been added. |
|
@DnMllr Do you have final thoughts on this topic? I don't think we can agree on this. Can @michaelobriena make a decision? |
|
@michaelobriena Can you make a decision please? I'm really against calling onMount on the component when adding a component (as outlined above). |
|
Are there any final thoughts on this? Can this be closed/ merged? @michaelobriena |
|
@alexanderGugel I don't think that a core Component class can have this much logic. I think it can have onAdd, onRemove, getId, and getNode. These are the core bits but stuff like requetUpdate and onUpdate don't make sense in a default case. |
|
@michaelobriena Currently |
fix: no default components on scene
Having a generic component base allows us to reduce redundancy in renderables and component. E.g. currently Mesh and DOMElement (and potentially all future renderables) have a _requestUpdate method. Abstracting this out into a core component encapsulates functionality commonly used by most components.
Currently the onMount abstraction is broken. onMount is being called in two completely different scenarios: 1. The component has been added to a node. 2. The node has been mounted. The same is true for onShow. This commit introduces two methods to solve this issue: 1. Component#onAdd 2. Component#onRemove
Getter method for Component#_requestingUpdate has deliberately not been added. Component#requestUpdate should be sufficient to hook into the node's update cycle.
alexanderGugel
force-pushed
the
Component
branch
from
620417b to
bea9978
Compare
|
Breaks after latest release. Looking into it. Probably should get in after #266 |
|
Closing. This is no longer relevant and doesn't really help. |
Having a generic component base allows us to reduce redundancy in
renderables and component. E.g. currently Mesh and DOMElement (and
potentially all future renderables) have a _requestUpdate method.
Abstracting this out into a core component encapsulates functionality
commonly used by most components.
@DnMllr I think we've been talking about this before. Thoughts?
@michaelobriena There is no specific Pivotal Tracker story attached to
this, but I think it's a needed refactor. There is a ton of redundancy
because of the lack of some sort of component base.