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

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

Closed
johanrd opened this issue Feb 29, 2024 · 3 comments
Closed

Comments

@johanrd
Copy link

johanrd commented Feb 29, 2024

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

@johanrd
Copy link
Author

johanrd commented Mar 8, 2024

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
Copy link
Collaborator

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.

@johanrd
Copy link
Author

johanrd commented Apr 22, 2024

@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,

@johanrd johanrd closed this as completed Apr 22, 2024
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

No branches or pull requests

2 participants