Component Manager Bounds #351

chadhietala · 2018-08-01T16:01:04Z

rwjblue

Awesome, thank you for putting this together @chadhietala!

Some things:

Can you update the bounds section to make it clear that the bounds object itself is stable?
Can you show a few examples of component templates, and show what bounds would be for them?

rwjblue · 2018-08-01T18:19:59Z

text/0000-Component-Manager-Bounds.md

+
+In [RFC#2013](https://github.com/emberjs/rfcs/blob/master/text/0213-custom-components.md#detailed-design) we outlined the component lifecycle and how a custom component is associated with a manager. This design builds off off of that existing work and concepts.
+
+### `elementHook` capability


elementHook doesn't really seem to fit with the other capability names. Suggestions:

elementAccess

domAccess

domAccessCallbacks

domCallbacks

elementCallbacks

bounds (I like this one best 😄)

Thoughts?

rwjblue · 2018-08-01T18:26:11Z

text/0000-Component-Manager-Bounds.md

+});
+```
+
+### `didRenderLayout` hook


I know that this is the name internally in the glimmer-vm, but I don't think its particularly relevant as a name.

Possibly: didCreateBounds?

Actually, can we avoid creating a new hook here and instead add bounds as a second argument to the already existing didCreateComponent hook? didCreateComponent is part of the asyncLifecycleCallbacks capability, and per the prior RFC is where didInsertElement should go.

The name seems much better, and the intent seems very much in line with what we are doing here...

I don't mind bike shedding about the name, but I don't think we want to bundle it with didCreateComponent. There may be situations that you need one but not the other, and we want to keep things as granular as possible. Plus, bundling the hooks forces their timing to be the same, which we probably don't want (and they are in fact different today).

Sounds good, I agree. Still would prefer another name than didRenderLayout it really doesn't feel cohesive with the other component manager method names.

rwjblue · 2018-08-01T18:27:47Z

text/0000-Component-Manager-Bounds.md

+
+  didRenderLayout(component, bounds) {
+    component.bounds = bounds;
+    component.didInsertElement();


Based on the prior RFC didInsertElement is more appropriately called in didCreateComponent (which was an optional asyncLifecycleCallbacks capability), I don't think we should recommend changing that...

In general, I think we need to recommend against calling any kind of user hooks here.

rwjblue · 2018-08-01T18:33:08Z

text/0000-Component-Manager-Bounds.md

+```
+
+### `didRenderLayout` hook
+Once the rendering engine has constructed the bounds for the component the `didRenderLayout` hook will be called with the `component` and a `Bounds` instance.


This phrasing is slightly off, the hook does not receive "the component" it receives the opaque result of calling createComponent (aka the "state bucket"). Different component managers may return the instance itself but it may also return some other POJO/state bucket.

rwjblue · 2018-08-01T18:34:59Z

text/0000-Component-Manager-Bounds.md

+
+In other words, components **should not** rely on the timing of DOM insertion outside of the component's own bounds.
+
+### `willDestroyLayout` hook


Assuming we can keep the bounds as a second arg to createComponent (one of my suggestions above), this can become willDestroyComponent...

rwjblue · 2018-08-01T18:35:59Z

text/0000-Component-Manager-Bounds.md

+
+### `willDestroyLayout` hook
+
+To balance `didRenderLayout` we will expose a hook called `willDestroyLayout` that is invoked during destruction. It will receive the `component` instance.


it doesn't get the component instance, it gets the "return value from createComponent" (may be a state bucket, may be a component instance)

rwjblue · 2018-08-01T18:36:58Z

text/0000-Component-Manager-Bounds.md

+type FirstNode = Node;
+type LastNode = Node;
+
+interface Bounds {


Bounds doesn't get .parentElement?

The interface in glimmer-vm does have parentElement property:

https://github.com/glimmerjs/glimmer-vm/blob/e30b3d64232a0972365c8cbb1dd8e236a0f33dcf/packages/%40glimmer/runtime/lib/bounds.ts#L4-L9

Due to the lack of timing guarantees of construction of the parent do think this actually useful?

I think we need to provide access to the parent. What we cannot guarantee is that the parent's children (i.e. your siblings) are fully constructed. (And possibly that the grandparent may not be construed yet.)

By the way, I think you meant to say:

interface Bounds { firstNode: Node | Bounds; lastNode: Node | Bounds; }

chadhietala · 2018-08-03T00:19:47Z

I think I incorporated the feedback.

rwjblue

Awesome, I'm 👍 here...

lennyburdette · 2018-08-08T16:56:41Z

Has anyone proposed a react-style “refs” api? Instead of relying on the framework to create special properties, we could have a declarative api that allows decorating the template with special attributes that create JavaScript-accessible node references.

Something like <div ref="a" /> to be accessed as this.refs.a in lifecycle hooks.

I’m just curious. Both APIs could exist simultaneously.

JCBarry · 2018-08-10T12:26:13Z

Not frequently, but I've had the need for this in more complex components and the workarounds aren't pretty or easy to test. 👍

chancancode · 2018-08-10T15:44:03Z

We discovered a problem with this design in the core team meeting last week. The intent of this is to offer functionality akin to classic component's this.element, didInsertElement and willDestroyElement. The common use case is to attach and cleanup jQuery or native event listeners.

On the surface, it seems like this gets the job done. However, consider the following component template:

{{#if this.isSelected}}
  <strong>selected</strong>
{{else}}
  <em>select me</em>
{{/if}}

Ignoring the whitespace nodes, the bounds object will point to the strong or em element depending on the component's this.isSelected state, which could change back and forth multiple times during the lifecycle of the component. Therefore, if you attached an event listener on the element at initial render time, you may not get a chance to detach it before the element goes away, thus leaking memory. Dereferencing the nodes referenced by the bounds and then saving them off or closing over them with a closure has the same issue.

Of course, in a sense this is no different than attaching event listeners directly to descendants of this.element, since they are also subject to change without notice. However, as there is a stable root element, the event listeners are almost always attached there in practice.

There is still a class of use cases that is somewhat compelling and does not have the problem. For example, a component may use the bounds to "autofocus" the first input element. This only involve accessing the bounds' content synchronously and does not require any cleanup. In general, these kind of "snapshotting" use cases are fine. However, they may call for more hooks, like one that runs after the bounds' content has (potentially) been updated so they could reapply the logic.

So we can adapt this proposal for that kind of "snapshotting" use cases, perhaps adding the update hook and possibly removing the teardown hook (cause it is not needed, and may be misleading). We can then explore alternatives for the original use case, perhaps a modifier/ref-based design or an additional capability that ensures (at runtime) that the component has a stable root element. However, my worry is that since the original use case is the most common and compelling one, having this API around without a solution for that use case will mislead our users and causing them to misuse this API.

chancancode · 2018-08-10T15:46:34Z

Relevant: emberjs/ember.js#12500

chadhietala · 2018-08-25T15:07:45Z

Closing in favor of #353

chadhietala and others added 2 commits August 1, 2018 11:59

Bounds RFC

dbda07b

Typo fixes...

7b26999

rwjblue reviewed Aug 1, 2018

View reviewed changes

chadhietala added 2 commits August 2, 2018 20:16

Updating based on feedback

f294452

fixup

4f541c3

rwjblue approved these changes Aug 3, 2018

View reviewed changes

chadhietala mentioned this pull request Aug 7, 2018

Implement Bounds API emberjs/ember.js#16869

Closed

tylerturdenpants mentioned this pull request Aug 9, 2018

Ember Times Issue 59 emberjs/website#3490

Merged

5 tasks

jayjayjpg mentioned this pull request Aug 9, 2018

blog(embertimes#59): add Component Bounds RFC writeup emberjs/website#3497

Merged

chadhietala closed this Aug 25, 2018

chadhietala mentioned this pull request Aug 25, 2018

Modifiers #353

Closed

rwjblue deleted the bounds branch January 23, 2019 16:35


		In [RFC#2013](https://github.com/emberjs/rfcs/blob/master/text/0213-custom-components.md#detailed-design) we outlined the component lifecycle and how a custom component is associated with a manager. This design builds off off of that existing work and concepts.

		### `elementHook` capability


		In other words, components should not rely on the timing of DOM insertion outside of the component's own bounds.

		### `willDestroyLayout` hook


		### `willDestroyLayout` hook

		To balance `didRenderLayout` we will expose a hook called `willDestroyLayout` that is invoked during destruction. It will receive the `component` instance.

Uh oh!

Component Manager Bounds #351

Component Manager Bounds #351

Uh oh!

Conversation

chadhietala commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chancancode Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chadhietala commented Aug 3, 2018

Uh oh!

rwjblue left a comment

Choose a reason for hiding this comment

Uh oh!

lennyburdette commented Aug 8, 2018

Uh oh!

JCBarry commented Aug 10, 2018

Uh oh!

chancancode commented Aug 10, 2018

Uh oh!

chancancode commented Aug 10, 2018

Uh oh!

chadhietala commented Aug 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chadhietala commented Aug 1, 2018 •

edited

Loading

chancancode Aug 1, 2018 •

edited

Loading