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

Fix updateNode being called with undefined rootNode #2

Merged
merged 1 commit into from
Sep 29, 2020
Merged

Fix updateNode being called with undefined rootNode #2

merged 1 commit into from
Sep 29, 2020

Conversation

simonihmig
Copy link
Owner

When redux dispatch() is called before getState() has ever been called, updateNode() (in the custom creatStore() function) will be called with rootNode still undefined.

This wasn't catched in tests as they coincidentally always called getState() (for the cache setup) before dispatching. The added test here reproduces the bug that I was seeing when that's not the case.

@simonihmig
Copy link
Owner Author

First of all I should actually say how great this is! 🚀

I assume this was influenced/triggered by the discussion you had with my teammate @lolmaus in RFC 566. Finally I found the time to try this out in our codebase (an internal shared addon) where we used ember-redux before, and the refactoring using tracked-redux seems to work really nicely (ignoring this little bug)! Especially the fact that memoization now works automatically for any nested props, whereas before using CPs the caching effect would not work at all even with their explicit dependencies because the immutable nature of the redux state would invalidate every time even if their dependencies have actually not changed.

This is now much better, so thanks a lot for your tireless effort to bring us all the benefits of autotracking! 😍

@simonihmig
Copy link
Owner Author

CI is failing for unrelated reasons, #3 should fix this.

@pzuraq
Copy link
Collaborator

pzuraq commented Sep 24, 2020

I believe the way that I'm patching the store is actually wrong, and can be done better with middleware. I just haven't had the time to actually dig in and fix it, unfortunately. That said, if this fixes the problem then I'm all for it

When redux `dispatch()` is called before `getState()` has ever been called, `updateNode()` (in the custom `creatStore()` function) will be called with `rootNode` still undefined.

This wasn't catched in tests as they coincidentally always called `getState()` (for the cache setup) before dispatching. The added test here reproduces the bug that I was seeing when that's not the case.
@simonihmig
Copy link
Owner Author

Rebased, green now.

@pzuraq pzuraq merged commit 8c35231 into simonihmig:master Sep 29, 2020
@simonihmig simonihmig deleted the fix branch September 29, 2020 07:35
@simonihmig simonihmig added the bug Something isn't working label Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants