{{component}} helper race condition with associated model

[trentmwillis]

The {{component}} helper seems to have a race condition that is causing problems to surface in tests that I am writing.

Usage is like so:

{{component card.componentType model=card}}

Then when tests are run, a failure occurs during the afterEach when destroy is run on the application instance.

If I change the above usage to use a static string rather than a model property like so:

{{component `card/my-type` model=card}}

The tests and teardown are fine. This leads me to believe it's being caused by the model being destroyed before the component, resulting in the component having a type of undefined.

Potentially related PR, #10302

[stefanpenner]

if possible, can you provide an repro that demo's this, maybe an ember-cli app that has this issue?

[jbrown]

@trentmwillis I've had issues with this as well, but I think the core issue is the component helper just doesn't handle undefined gracefully.

[lukemelia]

Related to #10302. Unsure how to move that PR forward.

[trentmwillis]

Got a reproduction working here: https://github.com/trentmwillis/ember-component-helper-issue

So it seems my initial assumption was wrong, our actual usage is more like:

{{component card.model.type ...}}

And so the issue appears to be with having nested model properties. I reproduced in the above repo with both fixture and http-mock data.

[trentmwillis]

Update: applying the solution given in #10302 didn't fix anything. I did track down that it is failing out here in the bound_component_view.

Since I don't quite understand streams and what read is hoping to accomplish I'm not sure I can offer a fix for this. I did note that changing line 33 to be:

var componentClass = this.componentClassStream.cache || read(this.componentClassStream);

Fixes the problem, but I don't think is a correct solution.

[trentmwillis]

So after digging into this more, I can't seem to reproduce without using Ember Data Models. So I'm thinking I should probably open a there instead.

[nathanhammond]

Can confirm, this only happens in the process of destroy. The {{component}} helper is rendering a component defined by a value in the store. When the store gets destroyed it propagates the newly undefined value into the {{component}} helper.

At that point the component tries to re-create itself (_createNewComponent) and fails because undefined isn't valid. This also explains why @trentmwillis only notices this failure when using Ember Data because otherwise the property isn't coming from the container in the same way.

Possible solutions:

• Catch undefined as an invalid name and fail gracefully instead of throwing.
• Enforce ordering in the container destroy.
• Tear down stream subscriptions as a first pass in destroying.
• Others?

[nathanhammond]

@lukemelia @stefanpenner Do you have a preference as to which way we solve this? It's blocking a few things and I'd like to go ahead and get a PR in.

[trentmwillis]

@nathanhammond thanks for looking into this! For what it's worth, I think enforcing ordering in the destroy makes the most sense. Mainly because having errors thrown in the case of undefined may still be useful in other situations.

[nathanhammond]

@trentmwillis The caveat is that most things in Handlebars render nothing when passed a falsey value, implying that false or undefined might should render nothing in the case of the {{component}} helper as well. Currently the {{component}} helper throws the same error reported here when directly passed false or undefined.

I'm not sure which approach is the solution with the "least surprise."

[krisselden]

@lukemelia @nathanhammond @trentmwillis so a couple of things here:

1. the whole point of destroy() being async is so that it can mark everything in the container as isDestroying before running teardown, something must be not checking for isDestroying. there is no point to view observers to execute during App.destroy().
2. updates in the view layer should go in tree order (in the App.destroy case still, this isn't the solution) but it is a general problem with the current view layer (a template below an if can update before the if is checked for example). when the glimmer branch lands, it will fix this issue as it always walks the tree for updates. until then, the other solution is to create a queue for view.registerObserver() and sort it by level.

[nathanhammond]

More information (notes for me): willDestroy is called after the stream subscribed to here is updated–meaning that isDestroying is not yet set.

[trentmwillis]

A thought, since isDestroying is not set on the stream, would it be possible to check for it on the container? I feel like that would be a simple solution until glimmer lands

[samselikoff]

I encountered something similar when doing

{{#if newContentModal}}
  {{component newContentModal
    lesson=lesson
    dismiss='dismissModal'}}
{{/if}}

When I set newContentModal back to null, I got the error. To work around this for now, I created a blank component (export default Ember.Component in /components/blank/component.js, no template) and set newContentModal to 'blank'.

[nathanhammond]

@samselikoff Did you feel dirty while doing it? ;) It seems like we should probably make the component helper not attempt to render a component with falsey values. Also, hard failures for missed lookups often seen in {{#each}} blocks over data we don't actually control seems like a foot shotgun.

@trentmwillis and I might take a swing at a safer component helper today... paging @lukemelia for thoughts.

[lukemelia]

@nathanhammond That seems like a reasonable first step to me. The solution that @krisselden suggested is more holistically correct but also much more involved.

[rwjblue]

Should be fixed as the test from #10302 is passing (as of Glimmer restructuring).

Happy to reopen if this is still an issue...