Skip to content
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

[BUGFIX release] Don’t force invalidation of block param streams #11570

Merged
merged 1 commit into from
Jun 28, 2015

Conversation

tomdale
Copy link
Member

@tomdale tomdale commented Jun 27, 2015

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:

{{! app/templates/components/block-with-yield.hbs }}
{{yield danger}}

(Here danger is a property on the component.)

Now we invoke it:

{{#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:

{{! 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

@tomdale tomdale force-pushed the 11519-block-param-infinite-loop branch from f53c254 to 33bb48f Compare June 27, 2015 17:59
@tomdale tomdale changed the title Don’t force invalidation of block param streams [BUGFIX release] Don’t force invalidation of block param streams Jun 27, 2015
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
@tomdale tomdale force-pushed the 11519-block-param-infinite-loop branch from 33bb48f to 2579522 Compare June 27, 2015 18:12
rwjblue added a commit that referenced this pull request Jun 28, 2015
[BUGFIX release] Don’t force invalidation of block param streams
@rwjblue rwjblue merged commit 2b6e4b6 into master Jun 28, 2015
@rwjblue rwjblue deleted the 11519-block-param-infinite-loop branch June 28, 2015 11:42
@denzo
Copy link

denzo commented Jun 29, 2015

This states that it has been merged into master. Does that mean it is available on the canary immediately?

@mmun
Copy link
Member

mmun commented Jun 29, 2015

@denzo Yes, usually a couple minutes after merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants