-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Glimmer] Late bound layouts #13588
[Glimmer] Late bound layouts #13588
Conversation
@@ -1,162 +0,0 @@ | |||
import { get } from 'ember-metal/property_get'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These don't makes any more sense with components.
template = owner.lookup('template:' + layoutName); | ||
} | ||
if (!template) { | ||
template = component.defaultLayout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a template factory too?
lateBound(template) { | ||
let definition; | ||
if (this._cache) { | ||
definition = this._cache[template.id]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given we create new instances from directly imported modules we likely should cache the instance we created for our env from the factory, otherwise the component will recreate and recompile everything if you directly import it onto layout.
☔ The latest upstream changes (presumably #13601) made this pull request unmergeable. Please resolve the merge conflicts. |
43086ca
to
3b16b63
Compare
QUnit.test('when wrapped in a template, precompile is the same as compile', (assert) => { | ||
// Simulating what happens in a broccoli plugin | ||
// when it is creating an AMD module | ||
let precompiled = template(precompile('Hello')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in an amd module, eval("require('template')("+precompile('Hello')+")")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should assert that precompile returns a string, and that when it is evaluated, it returns what template() expects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something around here was biting me w/ ember-performance
which is then eval'd?
Glimmer doesn't seem to expect the same treatment.
* Updating glimmer-engine WIP * [Glimmer2] Late bound layouts
In Ember we typically leverage convention over configuration for figuring out what component class goes with which template. That being said, today it's totally valid to opt-out of those conventions by importing the template into the class and setting it as the layout for the component. This PR re-introduces the support for this functionality. Depends on glimmerjs/glimmer-vm#179