jmurphyau/ember-truth-helpers

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

johanrd opened this issue · 3 comments

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)

// We use class-based helper to ensure arguments are lazy-evaluated
// and helper short-circuits like native JavaScript `||` (logical OR).
export default class OrHelper<T extends MaybeTruthy[]> extends Helper<

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

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.

Hi @johanrd! Thank you for reporting this! Could you please confirm you still see the issue after ember-intl/ember-intl#1848 got fixed and released?

As for class based helpers - you can see discussion when this decision was made #188 (comment) as that was the way to make Glint types work with generics.

@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 ember-intl/ember-intl#1848 and/or glimmerjs/glimmer-vm#1440.

Thanks also for the pointer to #188 (comment). 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,