glimmerjs / glimmer.js

Central repository for the Glimmer.js project
https://glimmerjs.com
MIT License
750 stars 75 forks source link

Accessing this.element throws exception for single root element with extra whitespace #77

Closed kevgathuku closed 2 years ago

kevgathuku commented 6 years ago

With the following template:

<div class="container">
  <h1>My App</h1>
</div>

Trying to access this.element in the didInsertElement Lifecycle Hooks throws an error:

Uncaught Error: The 'element' property can only be accessed on components that contain a single root element in their template. Try using 'bounds' instead to access the first and last nodes.

I found out that this is because of the newline at the end of the file, which is treated as a separate node. this.bounds.lastNodecontains a separate node with the content "↵" Even adding a singe whitespace character at the end of the div node makes this.element access fail.

This behavior seems kind of unintuitive, since according to my first impression of this, this should be considered to be a single root element. Would you say this is expected behavior, or is it recommended to always use this.bounds instead?


To reproduce: Add a single space immediately after the end of this template i.e. after </nav> https://github.com/glimmerjs/glimmer.js/blob/master/packages/%40glimmer/component/test/browser/element-test.ts#L50 Once you re-run the tests, the 'elements are supported' test fails.

rwjblue commented 6 years ago

Ultimately, I’m not certain we can solve this. In theory we could make this.element find the first element (and only fire an error when there is more than one element in the root), but that would require a decent amount of additional work for each access of this.element in a component so I’m not sure we should do it...

rwjblue commented 6 years ago

FWIW, I agree that the nuance here is pretty annoying. IMHO, this requires a bit more polish / thought. I vaguely believe having a discussion around this with @tomdale...

kevgathuku commented 6 years ago

Ultimately I was just confused by the error message, and I was just trying to figure out if I was doing something wrong. This is what led me down this rabbit hole. Right now, for most practical situations it seems this.element is not usable, since most editors insert a newline at the end of the file. Seems this.bounds is the way to go.

josemarluedke commented 6 years ago

I believe you can add a ...attributes to your template's root element and then the this.element will work as expected.

Example, if your template looks like this:

<div class="something"> 
  Something
</div>

It would become:

<div class="something" ...attributes> 
  Something
</div>
kevgathuku commented 6 years ago

@josemarluedke unfortunately that didn't work. It looks like that was disabled in this commit: https://github.com/glimmerjs/glimmer.js/commit/8f42d335784f4aaa7ee8ede83baa54261b4f2838

rjschie commented 5 years ago

I agree, this is very unintuitive (and annoying tbh), and difficult to find the problem. Would we lose anything by always having this.element return the same thing as this.bounds.firstNode? That should solve this problem in a performant way, not change functionality, and be a more intuitive issue to solve for those who perhaps meant to grab a different node.

I guess another option would be updating the template compiler to strip surrounding whitespace? Or does the bounds assert check happen before it's compiled? I'm not very familiar with how template compilation works, and where it fits into the pipeline.

rwjblue commented 5 years ago

In general,gimme.js will follow suit with Ember and remove this.element all together. The main blocker for this is adding proper element modifier support.