robwalkerco / redux-persist-filesystem-storage

Redux persist adaptor for React Native filesystem storage
MIT License
196 stars 71 forks source link

Install flow as dev dep, (quick) fix issues #42

Closed draperunner closed 5 years ago

draperunner commented 5 years ago

Since Flow is used in the source, it should be listed as a dev dependency in order to fix the correct version for contributors like myself.

I added a .gitignore file. I'd say all repos should have a .gitignore so people don't accidentally commit node_modules or other files that should'nt be committed.

I also added the yarn.lock file. From the examples in the README, it seems you prefer yarn over npm. Using a lock file is a good practice for ensuring that contributors install the exact same versions as specified.

Flow is also added as a package.json script and can be run with yarn flow or npm run flow.

Regarding the changes in index.js, I have done the minimum necessary to fix the errors. I didn't want to change the functionality of the code and maybe produce unwanted changes. But I have some questions/ideas to further improve the typings:

  1. FilesystemStorage.clear should either be part of the FilesystemStorage upon initialization, or all properties should be added after. The first is better, I'd say. When initializing an object with keys, Flow interprets the object as "sealed", and complains if adding new properties after, as done with clear.
  2. What's the point of returning the boolean in clear (second param of callback)? I had to add it since it wasn't part of the callback signature. But can clear be changed to returning void only? If you want to keep returning a boolean, you should also do it within the catch block for consistency. 2.1 The return type of the callback for getAllKeys was set to void, but since clear is returning a boolean in its callback for this method, errors appear. I set it to any.
robwalkerco commented 5 years ago

Thanks for the PR @draperunner

robwalkerco commented 5 years ago

Published to npm as v2.1.0