Implement component bounds feature
#19
Conversation
With the recent Glimmer VM changes to adopt IndustryStandardCase, Glimmer.js components now relax the requirement of a template having a single root element. In addition to having multiple elements, this also means that a template may have top-level nodes such as CommentNodes or TextNodes.
This change renders the previous `element` API incoherent because there is no guarantee of a single "main" element for a template. Instead, we now set the `bounds` property of a component, which contains pointers to the first node and last node.
This is obviously less convenient for cases where you do just have a single conceptual main template. For example, if you have a template with a single root element, you would probably prefer to use `this.element` instead of `this.bounds.firstNode` or `this.bounds.lastNode`.
We could automatically determine the "single element" case and assign element automatically, but we were afraid that it would become a refactoring hazard. If you had a template with a single root element, wrote a bunch of code that used `this.element`, and then someone came along and added a comment to the beginning, all of that code would suddenly stop working.
Instead, you can now indicate which element is the conceptual "main" element by using `...attributes`, which also is the location where invocation-side attributes will get splatted into.
For example:
```hbs
{{!-- MyComponent.hbs --}}
<h1>Title</h1>
<div class="content" ...attributes>
</div>
```
In this example, `this.element` would be the `<div>` element. If you invoked this component with `<MyComponent id="component-id" />`, this `<div>` would also receive the `id` attribute.
|
@tomdale 👏 The |
|
Clarifying question: do we need to always set |
|
@locks No, it is not required. You will get an error if you invoke a component with attributes and it doesn't have splattributes, and I want to add a development-time error that gives helpful guidance if you access |
|
Another question would be if Would |
|
@rtablada I don't believe this example works because attribute values can only be strings, and |
|
@tomdale cool. Good work. LMK if there's more discussion on how to handle passing down events. |
|
Just to clarify, I think some confusion will come down to this: If people expect @tomdale saying that " |
| * You should not try to access this property until after the component's `didInsertElement()` | ||
| * lifecycle hook is called. | ||
| */ | ||
| element: Simple.Element = null; | ||
| element: HTMLElement; |
There was a problem hiding this comment.
I don't fully understand this change. How does this work when rendering in Node-land?
There was a problem hiding this comment.
Accessing the DOM should only be done after didInsertElement(), which isn't invoked in Node-land.
There was a problem hiding this comment.
@tomdale - Ya, I don't really agree here. There are other hooks that are called where this.element might be used. A better solution (IMHO) is to pass element: HTMLElement in as an argument to didInsertElement. This preserves the proper typings when using this.element outside of didInsertElement, and allows usage inside didInsertElement without casting.
Also, generally speaking, this sort of discussion is super valuable and it is very unfortunate that it was not allowed before the PR was merged...
There was a problem hiding this comment.
We are trying to move away from having intermediate DOM in SSR mode entirely because of the performance impact. Sorry if you feel the PR was rushed. I wrote it with @chadhietala and reviewed it with @wycats so was definitely not sneaking in unvetted code.
|
@rtablada Not sure what you mean by "native" attributes vs. "string" attributes. All attributes are coerced to strings when being set: let div = document.createElement('div');
div.setAttribute('onclick', function() { });
div.getAttribute('onclick'); // => "function() { }"
typeof div.getAttribute('onclick'); // => "string" |
|
The multi root components and I'm seeying on the comments the usage of |
|
Did it? |
|
@cibernox Yes, this change was intentional. The use of CapitalComponents (and design of the Glimmer component API in general) dominated the agenda of our recent core team face-to-face meeting. 😄 We will have a write-up soon of the proposed new changes and their rationale. The TL;DR is that they unlock multi-root/fragment components in addition to being less cognitive overhead (you don't have to invent two words when one is the most descriptive). |
Introduce public dir for images, robot.txt, favicon, etc.
With the recent Glimmer VM changes to adopt IndustryStandardCase, Glimmer.js components now relax the requirement of a template having a single root element. In addition to having multiple elements, this also means that a template may have top-level nodes such as
CommentNodesorTextNodes.This change renders the previous
elementAPI incoherent because there is no guarantee of a single "main" element for a template. Instead, we now set theboundsproperty of a component, which contains pointers to the first node and last node.This is obviously less convenient for cases where you do just have a single conceptual main element. For example, if you have a template with a single root element, you would probably prefer to use
this.elementinstead ofthis.bounds.firstNodeorthis.bounds.lastNode.We could automatically determine the "single element" case and assign element automatically, but we were afraid that it would become a refactoring hazard. If you had a template with a single root element, wrote a bunch of code that used
this.element, and then someone came along and added a comment to the beginning, all of that code would suddenly stop working.Instead, you can now indicate which element is the conceptual "main" element by using
...attributes, which also is the location where invocation-side attributes will get splatted into.For example:
In this example,
this.elementwould be the<div>element. If you invoked this component with<MyComponent id="component-id" />, this<div>would also receive theidattribute.