Skip to content

Support async transforms #427

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

Closed
wants to merge 3 commits into from
Closed

Support async transforms #427

wants to merge 3 commits into from

Conversation

Noitidart
Copy link

I don't know if we want to support this, but i think the changes I made for this are correct.

@rt2zz
Copy link
Owner

rt2zz commented Aug 30, 2017

I am not against this, but I am concerned by a few things:

  1. is async await support prevalent enough to include at this point?
  2. what happens if the async transforms return out of order. You might stagedWrite an old state over a newer one. We probably need some sort of nonce to protect against this
  3. is there a perf cost to the IIFE? any reason not to simply make the parent callback async?
  4. do we need a timeout based write state trigger. Right now staged write waits for all keys to drain before actually writing out state, but async transforms create a risk that the pending key pool infrequently gets fully drained. This is the most performance critical and difficult to debug piece of redux-persist so I want to make sure we get this right.

@Noitidart
Copy link
Author

Thanks, I'm not sure about that IIFE perf cost. Doesn't this async await get transpiled out?

Yeah I totally am newbie at redux-persist, my implementation of async transforms is probably wrong, but I thought I covered it so that they don't return out of order.

@Noitidart
Copy link
Author

Noitidart commented Sep 18, 2017

As I have been using this over the last couple weeks, I think async transforms are super useful. It solves all my cases without me needing to modify the actual core. But I couldn't do it right and am not sure how to fix it up. Pretending if async transforms was done from scratch by someone who knows what they are doing, do you think async transforms will make it into v5 one day?

Should I close this PR as this for sure won't make it in? I wouldn't accept this PR that's why I'm saying :P

@rt2zz
Copy link
Owner

rt2zz commented Sep 20, 2017

:) I still want to figure this out, it does seem to have a lot of value, but also a lot of risk. Once the first release of v5 is out and stable I think will be easier to re-evaluate. As for this PR, I say we leave it open as a reminder to follow up.

@Noitidart
Copy link
Author

Thanks for that awesomely positive and hopeful comment!!

@rt2zz rt2zz closed this Oct 28, 2017
@aguynamedben
Copy link
Collaborator

Tracking in this issue: #303
And this PR: #360

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.

3 participants