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

refacotr: memorize getState and applyMiddleware #244

Closed
wants to merge 3 commits into from

Conversation

ariesjia
Copy link
Contributor

No description provided.

@ariesjia
Copy link
Contributor Author

ariesjia commented Apr 17, 2019

related to #164

@streamich streamich requested a review from wardoost April 17, 2019 11:28
Copy link
Contributor

@wardoost wardoost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unnecessary to memoize getState as state changes after every update. In this patch getState will always return the initial state which is not correct behaviour.

After removing the memoization I tried using this method of composing the middleware functions but it doesn't seem to work correctly. The addAndReset action use all middleware correctly. I don't fully understand why but reduxjs/redux#2145 is an interesting thread on this issue so I'll have a better look at that soon.

@ariesjia ariesjia closed this Apr 18, 2019
@ariesjia ariesjia reopened this Apr 18, 2019
@ariesjia
Copy link
Contributor Author

fixed getState always return the initial state issue , reducer will always return a new object , so we can memoize the getState method as state changes.

now the storybook demo works as previously

@wardoost
Copy link
Contributor

While I see memoizing getState/middlewareAPI is needed when using this hook in combination with other hooks this branch also suffers from the issue mentioned in #164 (comment).

@wardoost wardoost closed this Apr 20, 2019
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