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

Helper receiving arguments before being torn down #18969

Closed
mydea opened this issue May 13, 2020 · 12 comments
Closed

Helper receiving arguments before being torn down #18969

mydea opened this issue May 13, 2020 · 12 comments

Comments

@mydea
Copy link
Contributor

mydea commented May 13, 2020

I have a template-only component like this (slightly simplified):

{{#let (map-by @selectedRecipesWithColor 'id') as |recipeIds|}}
  {{#if (gt recipeIds.length 0)}}
    {{log 'CHECK' recipeIds}}
    {{#let
      (load-data-per-recipe
        recipeIds=recipeIds
      ) as |dataPromise|
    }}
      {{! ... do something }}

The {{load-data-per-recipe}} helper has an assertion that recipeIds is not empty. Looking at the code, it shouldn't be possible for this assertion to happen here. However, when removing the @selectedRecipesWithColor items until none remains, the assertion happens.

The "CHECK" log never appears, but the helper receives an empty array anyhow (and breaks on the assertion).

The parent component updates the selection in a simple action like this:

@action
updateSelectedRecipeIds(selectedRecipeIds) {
  this.selectedRecipesWithColor = selectedRecipeIds.map((id, i) => {
    return {
      id,
      color: chartColors[i],
    };
  });
}

It seems there is some sort of racing condition where the helper is called with new arguments when it should be torn down?

I am running 3.18.1!

@rwjblue
Copy link
Member

rwjblue commented May 21, 2020

Can you add some logging to map-by to see what is being passed and how it is recomputing? Also, what is the map-by helper that you are using here (something custom? From an addon? Etc)?

@mydea
Copy link
Contributor Author

mydea commented May 25, 2020

map-by is a custom, very basic helper:

export default helper(function mapBy([array, propertyName]) {
  return array.mapBy(propertyName);
});

But it is not really tied to this - it also happens when passing it to any other helper.

With a logging like this:

{{log 'A' @selectedRecipesWithColor}}
{{#if @selectedRecipesWithColor}}
  {{log 'B' @selectedRecipesWithColor}}
  {{#let (map-by @selectedRecipesWithColor 'id') as |recipeIds|}}
    {{! ... }}

&

export default helper(function mapBy([array, propertyName]) {
  console.log('C', array);
  return array.mapBy(propertyName);
});

I get the following output when first adding an item and then removing it:

// Add 
A [ {...} ]
B [ {...} ]
C [ {...} ]
// Now remove
A []
C []

@GavinJoyce
Copy link
Member

@mydea perhaps you can create a reproduction of this in a new ember app or an ember-twiddle?

I tried to recreate your issue here in 3.18.1, but it seems to work as expected:

https://ember-twiddle.com/51e2da469110db14d45757316f8df037?openFiles=templates.components.inner%5C.hbs%2Ctemplates.components.inner%5C.hbs

Screenshot 2020-05-28 at 09 14 17

@mydea
Copy link
Contributor Author

mydea commented May 29, 2020

OK, I finally managed to get a reproduction! It's a bit tricky, but you can see it:

https://ember-twiddle.com/146611a2343c1edd08f79fb5b0b8221c?openFiles=components.my-chart%5C.js%2Ctemplates.components.my-chart%5C.hbs

With this example, if you add one, them remove it, you will see that the helper is called again.

If I remove the {{did-update this.updateChartData @items}} part, the helper stops being called again.

@GavinJoyce
Copy link
Member

GavinJoyce commented May 29, 2020

Nice one on the reproduction. I'm seeing a similar behaviour in my app with {{did-update}} within an {{#if}} and was having trouble reproducing.

This looks like a bug to me

@mydea
Copy link
Contributor Author

mydea commented May 29, 2020

I played around with it a bit, and replace the {{did-update}} modifier with a very basic {{did-update}} helper, like this:

import Helper from '@ember/component/helper';

export default class DidUpdateHelper extends Helper {
  didRun = false;

  compute([fn]) {
    if (!this.didRun) {
      this.didRun = true;
      return;
    }

    fn();
  }
}

seemed to fix the issue. So it seems to somehow be connected with the modifier.

@NullVoxPopuli
Copy link
Contributor

I've also experienced this issue with a modifier deep in a component tree that shouldn't run, due to being wrapped in an if statement who's condition becomes false.

This seems like a teardown bug in the VM, yeah?
Like, components aren't torn down soon enough? Or Autotracking shouldn't be running on torn down templates? Idk

@NullVoxPopuli
Copy link
Contributor

Here is my reproduction, which I think is slightly different: https://ember-twiddle.com/5a3fa797b26e8807869340792219a5ee?openFiles=components.demo%5C.hbs%2Ccomponents.demo%5C.hbs

@NullVoxPopuli
Copy link
Contributor

I can obviously hack around it by adding guards in my getter in the sub-component, but I think the condition of the if statement becoming false should prevent the getter from ever re-evaluating with invalid data?

NullVoxPopuli added a commit to NullVoxPopuli/repro-repo-emberjs-18969 that referenced this issue Jun 14, 2020
@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Jun 14, 2020

Made a repro repo: https://github.com/NullVoxPopuli/repro-repo-emberjs-18969


Additional notes for other readers, my issue seems to be related to modifiers.

In the twiddle, if I change sub-component.hbs

-<span {{data-attributes-from this.derivedData}}>{{@data.kind}}</span>
+<span>{{@data.kind}} -- {{this.derivedData}}</span>

Then there is no error, and the subtree (as expected) doesn't re-render anything when the if condition becomes false.


Also this bug is present in 3.16.x

@NullVoxPopuli
Copy link
Contributor

Here is the entire reproduction in a single component: https://ember-twiddle.com/5a3fa797b26e8807869340792219a5ee?openFiles=components.demo%5C.hbs%2Ccomponents.demo%5C.hbs

it's weird, cause it is part of the spec (thanks @pzuraq !) https://github.com/emberjs/rfcs/blob/master/text/0373-Element-Modifier-Managers.md#destroymodifier

but this is very expected (to me) 🤔

when the scenario is all self contained, I can kinda get behind the behavior, but, when you have a couple layers of misdirection or abstraction in between, it is non-obvious, imo (I like obvious errors :D )

So... I'm not sure on a path forward here. Basically, anything passed to a modifier needs to optionally chain all property accesses as there is no way to guarantee the shape of data (afaict, anyway)

@andreyfel
Copy link
Contributor

@NullVoxPopuli I think this is fixed in 3.26 with #19469

@locks locks closed this as completed Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants