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.

Sign up for GitHub

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

Jump to bottom

Feature/use async storage #22

Merged
merged 11 commits into from
Feb 27, 2019
Merged

Feature/use async storage #22

merged 11 commits into from
Feb 27, 2019

Conversation

kadikraman
Copy link
Contributor

Fixes #16

Adding a useAsyncStorage hook to be used with React >=16.7.

@krizzu
Copy link
Member

krizzu commented Feb 26, 2019 •
edited
Loading

Hi @kadikraman, thanks for your work, looks great!

I'm going to hold off a bit before merging this one with few notes to consider:

  • Can you move this custom hook (and its type) to separate file (say lib/hooks.js) please? I'd like to not push everything into one file.

  • Since Hooks are a pretty new feature, I think having a separate doc explaining how to use it, with examples would be super useful for new developers

I'm going to assign issue and this PR to you. Sounds good?

@krizzu krizzu added the enhancement New feature or request label Feb 27, 2019
@kadikraman
Copy link
Contributor Author

Hi @krizzu - added a specific example and separated hooks.js into its own file. Had to also move AsyncStorage to a separate file so I could import it into hooks.js without causing a circular dependency, so the entry point is now index.js.

krizzu
krizzu requested changes Feb 27, 2019
View reviewed changes
Copy link
Member

@krizzu krizzu left a comment

Choose a reason for hiding this comment

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

Look great, thank You!

I left few comments to check out, let me know what you think.

docs/API.md Outdated
Uses the new [hooks api](https://reactjs.org/docs/hooks-intro.html) to give you convenience functions to get, set, merge and delete a value from a location.

The `useAsyncStorage` returns an object that exposes all all the methods that allow you to interact with the stored value.
Copy link
Member

Choose a reason for hiding this comment

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

all all typo here

docs/API.md Outdated
1. On mount, we read the value at `@storage_key` and save it to the state under `value`
2. When pressing on "update value", a new string gets generated, saved to async storage, and to the component state
3. Try refreshing the app with `cmd + R` - you'll see that the last value is still being read from async storage
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "Try to reload your app", to keep it platform-agnostic :)

docs/API.md Outdated
## `useAsyncStorage`

Uses the new [hooks api](https://reactjs.org/docs/hooks-intro.html) to give you convenience functions to get, set, merge and delete a value from a location.
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about changing from a location to from Async Storage, for given key or something similar? Just to point that this custom hook is Async Storage key-specific.

@kadikraman
Copy link
Contributor Author

@krizzu thanks for the feedback - updated.

@krizzu krizzu merged commit ef3c092 into react-native-async-storage:master Feb 27, 2019
@krizzu
Copy link
Member

krizzu commented Feb 27, 2019

Thanks a lot!

@kadikraman kadikraman deleted the feature/use-async-storage branch February 27, 2019 21:14
@krizzu
Copy link
Member

krizzu commented Mar 1, 2019

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@krizzu krizzu added the released label Mar 1, 2019
@vkurchatkin
Copy link

Could anyone elaborate, what's the point of this "hook"? It's not even a hook, really, since it doesn't use built-in hooks under the hood.

@krizzu
Copy link
Member

krizzu commented Mar 5, 2019

To be honest, having a second look at this, I get your point. I definitely see it's clearly missing a 'hook' functionality.

I'll add this to "clean up" list, to make this one actually use hooks API.

Thanks @vkurchatkin

@kadikraman
Copy link
Contributor Author

Yeah, my bad, it doesn't use the built in hooks under the hood. It's just a partially applied function that has a hook-like api.

Maybe a better idea would be to use useState to store the current value with some metadata for whether or not it has loaded 🤔

@krizzu
Copy link
Member

krizzu commented Mar 5, 2019

I've opened an issue for further discussion
#32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krizzu krizzu krizzu approved these changes

Assignees

@kadikraman kadikraman

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kadikraman @krizzu @vkurchatkin