Passing block param in component - modified twice in a single render #11519
Comments
|
Looks just like what I was seeing in #11277, @stefanpenner @rwjblue any ideas? |
|
We're seeing a more or less identical issue in our app. |
|
Same problem here. |
no need to ping me, i review every notifcation |
|
Sorry I wasn't sure. #11277 seemed to sneak into 1.13 even though I reported 28 days ago. |
|
@Samsinite I'm positive there are limited hands to review and fix bugs, so my guess is the core team prioritizes fixes. I'm guessing this means that not all issues get fixed in each 6-week release cycle. If I could make a suggestion, I did notice that your issue was perhaps harder to verify bc you had to clone a whole repo and run a project locally to test. Maybe that slowed the review process? |
|
Possibly, thanks for duplicating the issue into a jsbin. I am guessing that the core team is a little swamped right now when looking at the number of open issues on Github. Didn't mean to sound ungrateful as the ember core team does awesome work, just wanted them to be aware of the issue. |
|
@Samsinite no worries. want to close your issue and roll with this one with the jsbin? |
|
Sounds good, this one is newer so it likely has more visibility too. |
|
I'm having this same issue. |
|
TLDR; Using the variable being yielded in the component template causes a loop.. Using I looked into it and this is what I could find - might help someone with more experience solve this problem.. First Render:
Subsequent Renders (value changes):
Step 6 on rerender is where the issue starts. This call to Looks like this is because the component is subscribed to this particular stream, and it's subscribed to the stream because it's used in the template body of the component.. Changing the variable used in the component tempalte body to Commenting out this line fixes the issue - but I'm not sure what the side effect would be. Someone with more knowledge on HTMLBars would be able to best come up with a solution I think. This JSBin is the bare minimum needed to get this issue to occur. |
|
A better workaround is to create a computed property that is used just for yielding that produces the same value.. E.g:
|
|
Looking into this now. |
|
The issue is that Glimmer is re-using the yielded stream from the shadow scope instead of creating a proxy stream. When it re-validates the light scope template, it triggers a change notification on the block param stream. Unfortunately, this is the same stream from the shadow scope, and because |
var newValue = Stream.wrap(value, ProxyStream, key);I believe this line is the culprit. Reading this code, you may assume that the value is always wrapped in a |
|
I think the call to The question here is whether we need to create a new I can't come up with a reason why we have to force revalidation of the block param stream on re-renders, but maybe I'm forgetting something. It seems like a performance win if we can avoid it. |
This commit removes the forced invalidation of block param streams on
re-renders.
Previously, on re-renders, all block param streams were revalidated by
calling `notify()` on them blindly.
While this often works, it creates a vector for light scopes to
inadvertently invalidate their associated shadow scopes. In some cases,
this leads to an infinite loop.
For example, imagine a component with the following template:
```handlebars
{{! app/templates/components/block-with-yield.hbs }}
{{yield danger}}
```
(Here `danger` is a property on the component.)
Now we invoke it:
```handlebars
{{#block-with-yield as |dangerBlockParam|}}
{{dangerBlockParam}}
{{/block-with-yield}}
```
This works. The yielded block (light scope) receives a stream for the
`danger` value. On re-renders, the stream is invalidated, but all that
happens is the `{{dangerBlockParam}}` render node is dirtied again
(which doesn’t matter, because we haven’t rendered and cleaned it yet).
However, imagine we change the component template to now display the
same `{{danger}}` value:
```handlebars
{{! app/templates/components/block-with-yield.hbs }}
{{danger}}
{{yield danger}}
```
Now, when the block param stream is invalidated, it goes back and
dirties the render node _from the parent shadow scope_. Because the
component is dirtied, its `{{yield}}` helper is dirtied, causing the
block param streams to be dirtied, and so on in an infinite loop.
This commit removes the forced invalidation, which I believe is
unnecessary and probably left over from an earlier implementation.
Specifically, if we are yielded a stream (`{{yield danger}}`), the
stream should notify us of any changes to the value. Any downstream
consumers of that stream will be notified correctly.
In the case of `{{yield “hello”}}` or some other primitive, we already
wrap those values in a `ProxyStream` and call `setSource` when it
changes, which has the effect of invalidating the stream anyway.
Closes #11519
This commit removes the forced invalidation of block param streams on
re-renders.
Previously, on re-renders, all block param streams were revalidated by
calling `notify()` on them blindly.
While this often works, it creates a vector for light scopes to
inadvertently invalidate their associated shadow scopes. In some cases,
this leads to an infinite loop.
For example, imagine a component with the following template:
```handlebars
{{! app/templates/components/block-with-yield.hbs }}
{{yield danger}}
```
(Here `danger` is a property on the component.)
Now we invoke it:
```handlebars
{{#block-with-yield as |dangerBlockParam|}}
{{dangerBlockParam}}
{{/block-with-yield}}
```
This works. The yielded block (light scope) receives a stream for the
`danger` value. On re-renders, the stream is invalidated, but all that
happens is the `{{dangerBlockParam}}` render node is dirtied again
(which doesn’t matter, because we haven’t rendered and cleaned it yet).
However, imagine we change the component template to now display the
same `{{danger}}` value:
```handlebars
{{! app/templates/components/block-with-yield.hbs }}
{{danger}}
{{yield danger}}
```
Now, when the block param stream is invalidated, it goes back and
dirties the render node _from the parent shadow scope_. Because the
component is dirtied, its `{{yield}}` helper is dirtied, causing the
block param streams to be dirtied, and so on in an infinite loop.
This commit removes the forced invalidation, which I believe is
unnecessary and probably left over from an earlier implementation.
Specifically, if we are yielded a stream (`{{yield danger}}`), the
stream should notify us of any changes to the value. Any downstream
consumers of that stream will be notified correctly.
In the case of `{{yield “hello”}}` or some other primitive, we already
wrap those values in a `ProxyStream` and call `setSource` when it
changes, which has the effect of invalidating the stream anyway.
Closes #11519
(cherry picked from commit 2579522)
This commit removes the forced invalidation of block param streams on
re-renders.
Previously, on re-renders, all block param streams were revalidated by
calling `notify()` on them blindly.
While this often works, it creates a vector for light scopes to
inadvertently invalidate their associated shadow scopes. In some cases,
this leads to an infinite loop.
For example, imagine a component with the following template:
```handlebars
{{! app/templates/components/block-with-yield.hbs }}
{{yield danger}}
```
(Here `danger` is a property on the component.)
Now we invoke it:
```handlebars
{{#block-with-yield as |dangerBlockParam|}}
{{dangerBlockParam}}
{{/block-with-yield}}
```
This works. The yielded block (light scope) receives a stream for the
`danger` value. On re-renders, the stream is invalidated, but all that
happens is the `{{dangerBlockParam}}` render node is dirtied again
(which doesn’t matter, because we haven’t rendered and cleaned it yet).
However, imagine we change the component template to now display the
same `{{danger}}` value:
```handlebars
{{! app/templates/components/block-with-yield.hbs }}
{{danger}}
{{yield danger}}
```
Now, when the block param stream is invalidated, it goes back and
dirties the render node _from the parent shadow scope_. Because the
component is dirtied, its `{{yield}}` helper is dirtied, causing the
block param streams to be dirtied, and so on in an infinite loop.
This commit removes the forced invalidation, which I believe is
unnecessary and probably left over from an earlier implementation.
Specifically, if we are yielded a stream (`{{yield danger}}`), the
stream should notify us of any changes to the value. Any downstream
consumers of that stream will be notified correctly.
In the case of `{{yield “hello”}}` or some other primitive, we already
wrap those values in a `ProxyStream` and call `setSource` when it
changes, which has the effect of invalidating the stream anyway.
Closes #11519
(cherry picked from commit 2579522)
|
Rad, thanks guys! This will go in a 1.13.x release before 2.0 right? |
|
@Samsinite - Yes, absolutely. |
|
I can confirm that this solved our issue! Thanks @jmurphyau and @tomdale ⛵ |
I'm not sure if this is related to #11277, but I'm experiencing a similar issue. I've been able to reproduce the issue on this jsbin.
The first two uses of {{editable-thing}} work fine, but the last where we pass through the
isEditingparam via yield starts the unending warning loop (fair warning, open Chrome's task manager so you can kill the "runaway process" when you click the third edit button).Is this a bug or a misuse/misunderstanding of block params? Either way, would love to understand this better.
The text was updated successfully, but these errors were encountered: