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

Fixed for supporting flow #4

Merged
merged 2 commits into from Feb 14, 2019

Conversation

ifsnow
Copy link
Contributor

@ifsnow ifsnow commented Feb 12, 2019

It is the same as the PR that I submitted in react-native repository.

I hope this helps.

@cpojer
Copy link
Contributor

cpojer commented Feb 12, 2019

Wow, nice. Thank you! I'll leave it to @krizzu to review and merge this PR. :)

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.

Thanks for the first contribution 💪
I've left one suggestion to consider, let me know what you think.

@@ -21,6 +20,21 @@ const RCTAsyncStorage =
NativeModules.RNC_AsyncSQLiteDBStorage ||
NativeModules.AsyncLocalStorage;

type ReadOnlyArrayString = $ReadOnlyArray<string>;
Copy link
Member

Choose a reason for hiding this comment

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

What about exporting this to different file, say types/flow.js and then include it in .flowconfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krizzu
There is some troubles when using .flowconfig. For example, hyperlink(Go to definition) in vscode does not work properly.

I think the approach below looks ok.

  • Added AsyncStorageType.js to define types and separated it from AsyncStorage.js.
  • Added AsyncStorageType.js.flow to provide more clean autocomplete.

Let me know what you think about this.

Copy link
Member

Choose a reason for hiding this comment

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

On the second thought, having types defined in the same file is better than have 2 different files with types. Thought extracting flow types into another file would be better supported by VScode.

If you would revert 1 commit back, I'd be more than happy to merge it 👍
Sorry for confusion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krizzu Sure. I had a similar idea. I modified it as you said.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thank You.
In that case, where we have Types in the same file, do we still need .js.flow file? Any advantages to having it?
Also, can you rebase with master, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krizzu With AsyncStorage.js.flow, we can use more organized auto-completion in VSCode, but I realized there was a problem with the function to jump directly to AsyncStorage.js to check the code.

It seemed to have more disadvantages than merits, so I decided to delete AsyncStorage.js.flow. Sorry for confusion!

I finished what you said.

@cpojer cpojer mentioned this pull request Feb 14, 2019
64 tasks
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.

Thanks!

@krizzu krizzu merged commit 4d30fc2 into react-native-async-storage:master Feb 14, 2019
tobyworks pushed a commit to my-channels/react-native-async-storage that referenced this pull request Jul 9, 2020
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.

None yet

3 participants