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 leak fix in decorator implementation. #410

Merged
merged 2 commits into from
May 30, 2017

Conversation

SethDavenport
Copy link
Member

@SethDavenport SethDavenport commented May 30, 2017

Fix for angular-redux/example-app#34, thanks to @aminpaks for the fix. Reconstituted from #409, with an additional commit from me to remove unintentionally-committed build output.

Amin Pakseresht and others added 2 commits May 29, 2017 21:48
- Fixes memory-leaks in NgRedux select decoratos that instantiate and return new observable everytime getter is called
@SethDavenport SethDavenport merged commit 7a5c73b into angular-redux:master May 30, 2017
@SethDavenport SethDavenport deleted the fix/mem-leak branch May 30, 2017 01:56
@DcsMarcRemolt
Copy link
Contributor

Just fyi, this change broke tests in my application. I was subscribing to a @select decorator of a component inside the tests, firing actions and testing if the state changes correctly via selector.

As a good citizen I reset the store and everything else before every test.

This setup now breaks as the observable (var result) is cached at "compile time" (I know there is not real compile time in JS) hard binding to the initial store ignoring the swapping out of the store in my tests.

I worked around this by using store.select() instead of the decorator.

This should imho have been a breaking change, not a patch level one.

@SethDavenport
Copy link
Member Author

see issue #413

@DcsMarcRemolt
Copy link
Contributor

Didn't see that one, sorry. Thanks for the hint!

@SethDavenport
Copy link
Member Author

No worries - it wasn't supposed to be a breaking change 😳

I've released 6.4.2 which fixes the regression.

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

Successfully merging this pull request may close these issues.

2 participants