search

tannerntannern / ts-mixer

A small TypeScript library that provides tolerable Mixin functionality.
MIT License
379 stars 27 forks source link

Memory leak in mixin tracking #42

Closed Artaud closed 6 months ago

Artaud commented 2 years ago

The fact that mixins are being tracked globally in a map leads to a memory leak as the mixins are never unregistered from the map, preventing garbage collection of the respective classes.

https://github.com/tannerntannern/ts-mixer/blob/2e433b43d8c332d35222a656fcc78e08b0a83716/src/mixin-tracking.ts#L5

Simple solution would be to use a WeakMap.

Artaud commented 2 years ago

Also - regarding this design decision - why are mixins tracked in a global map? Wouldn't it be more handy to add some metadata onto a class mixin is applied to?

tannerntannern commented 2 years ago

Thanks for raising the issue. WeakMap indeed seems like a simple fix. I'd happily accept a PR! Ideally we'd also want a mocha test that verifies that the memory leak doesn't happen anymore, which I'm not sure how to do.

Regarding the design decision -- I honestly don't remember 😅. I think it has something to do with the fact that hasMixin(instance, mixin) doesn't actually require mixin to be a class created by ts-mixer, or some other fact about the implementation that requires hasMixin to know about all mixins. I could be mistaken, but I don't remember what my thinking was at the time.

Battosuai commented 7 months ago

Can I create a pr with WeakMap in mixin-tracking?

tannerntannern commented 7 months ago

Sure!

tannerntannern commented 6 months ago

Released in v6.0.4. Thanks @Battosuai for the contribution!