Conversation
There was a problem hiding this comment.
This is causing some tests to randomly fail with this assertion. The default tagName for all components is intentionally null, and that gets converted into a 'div' later down the road. I don't know how this is not causing a lot more tests to fail – it might indicate a deeper problem, e.g. we are accidentally leaking tagName: '...' onto the prototype, etc.
wycats
force-pushed
the
mut
branch
11 times, most recently
from
4c61704 to
3e84bbd
Compare
In a few places, we are incorrectly passing a function to `assert`. Since the `assert` function does not actually take a function, it just treats it as a truthy input, hence these asserts never actually fired. This error is mostly masked by the fact that we are also using the wrong Component class in some of the Glimmer tests. This also removes the aria-role from the default `attributeBindings` for curly components in Glimmer. Instead, it is implemented in a lower-level hook, similar to how `elementId` works. This allows us to avoid parsing the `attributeBindings` micro-syntax in most components and should make component creation a tiny bit faster.
| action = action[INVOKE]; | ||
| } else if (actionType !== 'function') { | ||
| throw new EmberError(`An action could not be made for \`${rawAction}\` in ${target}. Please confirm that you are using either a quoted action name (i.e. \`(action '${rawAction}')\`) or a function available in ${target}.`); | ||
| // TODO: Is there a better way of doing this? |
There was a problem hiding this comment.
Did you leave this TODO in on purpose? Is there a better way to do this? 
There was a problem hiding this comment.
This actually was in reference to the line below the todo:
let rawActionLabel = rawActionRef._propertyKey || rawAction;
There's no guarantee, I think, that rawActionRef is a propertyReference, and accessing a private property on the reference anyway is problematic.
| class MutableCell { | ||
| constructor(ref, value) { | ||
| this[MUTABLE_CELL] = true; | ||
| this._ref = ref; |
There was a problem hiding this comment.
Can you make this a symbol too? The MutableCell should have only a .value property and update method. I do not want to expose the internal reference if we can avoid it...
|
Left a few nit-picks, but this LGTM once those are addressed... |
Also, implement support for the [INVOKE] symbol in closure actions.
This also enables (partial) "reference pooling" on components – when
doing rending a `{{foo}}` in the `{{foo-bar}}` layout, it first checks
if `{{foo}}` is an arg passed from the outside (`{{foo-bar foo=...}}`),
and if so, it tries to reuse the same reference instead of making a new
one. This should reduce the total number of references created and also
reduce the number of `value()` computation as the same reference can be
reused across multiple curlies.
This also allows Glimmer to better optimize things – for example, if the
component is invoked with literals (`{{foo-bar foo=true}}`) and `foo` is
then used in a conditional in its layout (`{{#if foo}}...`), Glimmer
will be now able to see that `foo` is a const reference and optimize out
the updating step. This works recursively as well – if `foo-bar` then
passes the same property down (`{{bar-baz bar=foo}}`), we now propagate
the same reference all the way down.
Because everything in the new (curly) Component implementation is fully
two-way-bound by default, this does not actually require the `{{mut}}`
helper.
3 participants
Also, implement support for the [INVOKE] symbol in closure actions.
This also enables (partial) "reference pooling" on components – when doing rending a
{{foo}}in the{{foo-bar}}layout, it first checks if{{foo}}is an arg passed from the outside ({{foo-bar foo=...}}), and if so, it tries to reuse the same reference instead of making a newone. This should reduce the total number of references created and also reduce the number ofvalue()computation as the same reference can be reused across multiple curlies.This also allows Glimmer to better optimize things – for example, if the component is invoked with literals (
{{foo-bar foo=true}}) andfoois then used in a conditional in its layout ({{#if foo}}...), Glimmer will be now able to see thatfoois a const reference and optimize out the updating step. This works recursively as well – iffoo-barthen passes the same property down ({{bar-baz bar=foo}}), we now propagate the same reference all the way down.Thanks @Joelkang for doing most of the work on this one in #13541!
Closes #13541