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

Is it possible to create an async transform? #303

Open
cascornelissen opened this issue Mar 21, 2017 · 14 comments · May be fixed by #360
Open

Is it possible to create an async transform? #303

cascornelissen opened this issue Mar 21, 2017 · 14 comments · May be fixed by #360

Comments

@cascornelissen
Copy link

cascornelissen commented Mar 21, 2017

Looking through the listed transforms I've noticed redux-persist-transform-encrypt has an async version but it appears to simply wrap the sync one and the README has the following note:

Asynchronous support is still a work in progress.

Any plans on supporting async (Promise-based) transforms in the (near) future?

@cascornelissen
Copy link
Author

@rt2zz, I would love an answer on this as I will have to write my own storage solution or submit a PR if this isn't supported in redux-persist. Thanks in advance!

@rt2zz
Copy link
Owner

rt2zz commented Apr 2, 2017

@cascornelissen sorry for the delay. Currently async transforms are not supported. I am open to the idea but we would need to be very careful around performance and also error handling.

What is the specific need that is async? Is it possible you can bake the async portion into the storage engine (which is already async, namely getItem and setItem)

@cascornelissen
Copy link
Author

@rt2zz, I'm trying to transform data via SubtleCrypto before storing it.

I started working on something but haven't had the time to dive into it any deeper. You might want to give me some pointers/tips though, I've only implemented a working inbound transform so far: master...cascornelissen:master

@rt2zz
Copy link
Owner

rt2zz commented Apr 4, 2017

nice, approach seems reasonable. I am still not sure how this feature fits in, given that this complicates the common case. I am working on some lower level refactor atm, so for the time being I think the best options are either

  • keep rolling with your fork
  • implement the async encrpytion in the storage engine

@cascornelissen
Copy link
Author

cascornelissen commented Apr 4, 2017

Given the fact that I want to use multiple storage engines based on the environment later on I'll go with option 1 for now. Could you let me know when this refactor is finished (and do you have any ETA)? I might want to submit a PR at that time to implement the Promise-based transforms if anyone's interested.

For the argument saying that it complicates the common case: I agree, maybe we could make this configurable behind a flag? But we'll see once your refactor is finished.

@CharlieHess
Copy link

@cascornelissen I actually need the same thing; did you keep rolling with your fork (and is it published anywhere?)

@CharlieHess CharlieHess linked a pull request May 28, 2017 that will close this issue
@CharlieHess
Copy link

@rt2zz I went ahead and implemented it in #360. I have a pretty well-defined use case (a reducer that is used very infrequently, but needs to imbue state with passwords from https://github.com/atom/node-keytar) and in my testing there is no perf impact.

@cascornelissen
Copy link
Author

@CharlieHess, unfortunately not, some more important work came in but I love what you've been working on so far!

@aguynamedben
Copy link
Collaborator

@CharlieHess I was going to mark this issue stale since the issue hasn't been updated in a long time, but it looks like your PR is still outstanding and you're still seeking feedback now that v5 is out.

Discussion in here: #360

Is this still something you'd like a resolution on? Thanks for submitting your PR to the project. @rt2zz seemed somewhat concerned about the support burden of this PR, and he's already stretched in supporting this project. It would help the project immensely if we could get a commitment that you'd help support this feature and address any issues that come up with it.

@aguynamedben
Copy link
Collaborator

Tagging a few of the other people who seem interested in this feature from PR comments: @antondomratchev, @cascornelissen, @jstaab, @mnquintana, @sebastian-schlecht

@aguynamedben
Copy link
Collaborator

@dahannes, @dhruvparmar372, and @mairi17 also seems interested in the PR based on 👍's. My project also stores things in keytar (async) that I wish would be blacklisted and rehydrated. So there seems to be around 9-10 people wanting this, including people who work at Slack and legit companies.

That is @CharlieHess stated use case:

In my case I wanted to store secrets securely (say, in the keychain) and have them excluded from the persisted shape. Then, on rehydrate, I wanted to read the secrets from the keychain and apply them to the state as if they were there all along.

@CharlieHess
Copy link

@aguynamedben sooo, at slack we've pretty much stuck with my fork for the time being, but eventually we'll have to upgrade from v4 => v5 and then I'll come back to this. I can't see that happening in the near-term though. 😔

@agonzalezjr
Copy link

Any updates or path forward on this?

@aguynamedben
Copy link
Collaborator

FWIW we just moved to an "as simple as possible" solution as recommended by Dan Abramov here: https://egghead.io/lessons/javascript-redux-persisting-the-state-to-the-local-storage. We have setInterval save state to localStorage every 1000ms. It works nicely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants