jmurphyau / ember-truth-helpers

Ember HTMLBars Helpers for {{if}} & {{unless}}: not, and, or, eq & is-array
MIT License
706 stars 95 forks source link

Memory leaks with `and ` and `or` after they were converted to class based helpers #205

Closed johanrd closed 5 months ago

johanrd commented 7 months ago

After long nights of debugging memory leaks, I found an issue with the and and or helpers from ember-truth-helpers, after they were converted to class based helpers. E.g. when using them with ember-template-imports:

import RouteTemplate from 'ember-route-template';
import { or } from 'ember-truth-helpers'

export default RouteTemplate(<template>
  <ProjectList as |projectList|>
    <EquimpentList as |equipmentList equipmentConnection|>
      <Dashboard @isLoading={{or projectList.isLoading equipmentList.isLoading}} />
    </EquimpentList>
  </ProjectList>
</template>);

Here ProjectList and EquimpentList yields a resource, similar to reactiveweb's RemoteData or glimmer-apollo's useQuery.

Components inside the Dashboard that consume isLoading are simply retained in memory after exiting the route, and multiplying in memory every time the route is re-entered.

Converting to a pure functional helper fixed the leak. Also – as proof of concept – the following use of double not-helper also fixed the leak (as the not-helper is still a pure function helper)

<Dashboard
-  @isLoading={{or projectList.isLoading equipmentList.isLoading}}
+  @isLoading={{not (not projectList.isLoading) (not equipmentList.isLoading)}}
</Dashboard>

@SergeAstapov I am not 100% sure about what runtime problem / use case the the change to class based helper solved, so therefore I'm also not sure whether the change is worth it. (yet the compile time typescript generics should be solvable without needing to convert to a class, I think)

https://github.com/jmurphyau/ember-truth-helpers/blob/93ba8bc7e96422c88c60d0c43469ed166e0a163a/packages/ember-truth-helpers/src/helpers/or.ts#L25-L27

Posting the issue now, without a link to a reproducible repository, in case others need a hint of the issue before I pull together something more reproducible.

best regards, johan

johanrd commented 7 months ago

Okay, so an update here: I have yet to create a minimal reproduction. It might be linked to happening only in-combination with a memory leak regression i found in ember-intl.

That said, when Class based helpers are being considered a legacy feature (according to https://github.com/chancancode/ember-serviceable-helper?tab=readme-ov-file#motivation), and since (some sort of) ember-truth-helpers are considered for inclusion in ember by default (according to https://polaris-starter.pages.dev/), I think this repository should strive for being pure functions only.

SergeAstapov commented 6 months ago

Hi @johanrd! Thank you for reporting this! Could you please confirm you still see the issue after https://github.com/ember-intl/ember-intl/issues/1848 got fixed and released?

As for class based helpers - you can see discussion when this decision was made https://github.com/jmurphyau/ember-truth-helpers/pull/188#discussion_r1310801620 as that was the way to make Glint types work with generics.

johanrd commented 5 months ago

@SergeAstapov Thanks for your reply. Closing for now, as I have yet to pin it to a simple reproduction. It may have been related to https://github.com/ember-intl/ember-intl/issues/1848 and/or https://github.com/glimmerjs/glimmer-vm/pull/1440.

Thanks also for the pointer to https://github.com/jmurphyau/ember-truth-helpers/pull/188#discussion_r1310801620. It surely does not feel right to use class based helper in such a 'plain' function, but if the only other option is to embed it into the vm, I think it can wait.

thanks,