-
Notifications
You must be signed in to change notification settings - Fork 190
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
Failing test for forwarding attributes to yielded components #892
Failing test for forwarding attributes to yielded components #892
Conversation
}, | ||
{} | ||
); | ||
debugger; |
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.
ooops
@chancancode did you have time to check what might be wrong with this? |
Remove wrapper div (and double splatting).
b0886b7
to
29099c2
Compare
Prior to the changes here, for every `AttrNode` we processed we processed its value (handling literals, mustaches, etc). The value we process is then consumed by whatever operation is represented (e.g. `staticArg`, `dynamicArg`, `staticAttr`, `trustingAttr`, `dynamicAttr`). Unfortunately, when we processed the `...attributes` `AttrNode` into an `attrSplat` we did not also ensure to consume its value (which we don't really care about). The result was that we had one too many literal values on the `JavascriptCompiler`'s `values` array, meaning _all_ other operations that use a value would be getting the _prior_ operations value (e.g. an "off by one" issue in the values array). The fix here is to ensure that `attrSplat` consumes the value that is pushed to ensure things stay balanced.
29099c2
to
a233e05
Compare
I've just pushed a fix, here is the explanation from the commit message:
|
@rwjblue great. Can we merge this? How do updates to the glimmer-vm roll out to ember versions? Could Ember 3.7.1 get this? |
Will need to be backported for inclusion in Ember 3.4 LTS, 3.7, and 3.8+ into the following branches:
|
Extracted from emberjs/ember.js#17146
@chancancode @rwjblue I hope this reproduction helps track this down.
In the test, as it stands right now, the
<Inner ...attributes>
component is not rendered at all. If you remove the...attributes
it does render.