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

Concurrent mode? #15

Closed
dai-shi opened this issue Mar 27, 2019 · 7 comments · Fixed by #26
Closed

Concurrent mode? #15

dai-shi opened this issue Mar 27, 2019 · 7 comments · Fixed by #26

Comments

@dai-shi
Copy link
Owner

dai-shi commented Mar 27, 2019

From the comment by @MrWolfZ : reduxjs/react-redux#1179 (comment)

it makes render impure since through the proxy it writes to a mutable reference (yes, it only assigns ref.current inside effects but e.g. trapped.reset() etc. during render mutate the objects inside those references); that said, while this means it is not strictly pure, all mutations your library currently performs are "predictable" and reproducible, so the render still has the pure property of same inputs = same outputs

That's what I thought. Not technically pure, but reproducible. So, flushing rendering results in concurrent mode is fine.
Note: WeakMaps are there for performance reasons. It works without them.

Final note regarding concurrent mode: As @markerikson noted in a source code comment, I believe as well that any solution which accesses store.getState() during render will have issues with concurrent mode, which includes your library.

That's something I thought doubtful too. Let me learn about it.

@theKashey
Copy link

Don't forget - many of these issues, including the original state tearing one - are theoretical.

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 27, 2019

@MrWolfZ Where can I find a source code comment @markerikson noted?

@MrWolfZ
Copy link

MrWolfZ commented Mar 27, 2019

@dai-shi it's here

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 27, 2019

Thanks! Quoting it again.

https://github.com/reduxjs/react-redux/blob/79982c912d03c55ac7059b61806a1387352e91f3/src/components/connectAdvanced.js#L271-L274

// TODO We're reading the store directly in render() here. Bad idea?
// This will likely cause Bad Things (TM) to happen in Concurrent Mode.
// Note that we do this because on renders _not_ caused by store updates, we need the latest store state
// to determine what the child props should be.

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 29, 2019

I don't think I fully understand the comment. Maybe, we need to ask @markerikson, who's obviously busy...


We can't conclude anything until we know what the final concurrent mode is like, but if the store state is updated within a single batchedUpdates process, it will break for sure. If that's the case, we would need to lock the store while running the batched Updates and put dispatches in a pending queue.

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 29, 2019

We don't need to lock the store, but just keep the state for updates.
5fde761


Oh no, it doesn't work, because we don't know when exactly render functions are called.

So, I think I now understand @markerikson 's comment. Unless batchedUpdates run render functions synchronously, reading from store.getState() is not safe. But, then, it can't be helped anyway until we can pass the entire state as a context.

Or, if React provices a callback for batchedUpdates, that would help.

dai-shi added a commit that referenced this issue Mar 29, 2019
@dai-shi
Copy link
Owner Author

dai-shi commented Mar 29, 2019

Something like this: 1191f77

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 a pull request may close this issue.

3 participants