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

multiGet return type incorrect? #764

Closed
5 tasks
mtt87 opened this issue Mar 8, 2022 · 5 comments · Fixed by #767
Closed
5 tasks

multiGet return type incorrect? #764

mtt87 opened this issue Mar 8, 2022 · 5 comments · Fixed by #767
Labels
bug Something isn't working

Comments

@mtt87
Copy link
Contributor

mtt87 commented Mar 8, 2022

What happened?

👋 while using the library I noticed that multiGet has this signature:

multiGet: (keys: string[], callback?: MultiGetCallback) => Promise<readonly KeyValuePair[] | void>;

I'm not sure the return type is correct

Promise<readonly KeyValuePair[] | void>

It seems like void doesn't make sense, if I understood correctly how it works when a value doesn't exist it will return [string null] so the KeyValuePair[] is always the return type, not sure in which scenario it would return void.

I noticed it because Typescript is not happy when doing this

const values = await AsyncStorage.multiGet([ KEY1, KEY2, KEY3 ]);
setSomething(values[0][1]);

Showing this type error

Element implicitly has an 'any' type because expression of type '0' can't be used to index type 'void | readonly KeyValuePair[]'.
  Property '0' does not exist on type 'void | readonly KeyValuePair[]'.

Version

1.16.1

What platforms are you seeing this issue on?

  • Android
  • iOS
  • macOS
  • Windows
  • web

System Information

n/a

Steps to Reproduce

n/a

@mtt87 mtt87 added the bug Something isn't working label Mar 8, 2022
@tido64
Copy link
Member

tido64 commented Mar 8, 2022

I agree it's a bit weird. void doesn't really exist in JS as it would simply return undefined, or null in this case. In TypeScript, it's fine to use void for a function that may not return anything. The fix to your example is to add a check:

const values = await AsyncStorage.multiGet([ KEY1, KEY2, KEY3 ]);
if (values) {
  setSomething(values[0][1]);
}

This will allow TypeScript to narrow the type to KeyValuePair[] (since values cannot be void inside the if-statement), and you'll be able to index is as normal.

@mtt87
Copy link
Contributor Author

mtt87 commented Mar 8, 2022

@tido64 thanks for your reply

In TypeScript, it's fine to use void for a function that may not return anything.

Sure but is this really the case? Can this function not return anything?
It's 2 different things to not return anything (void or undefined or null) or return an array of KeyValuePair that has [string, null] when the key is not in the storage or [string, string] when the storage has the key.

What I'm saying here is that the library should probably change

Promise<readonly KeyValuePair[] | void>

to

Promise<readonly KeyValuePair[]>

@tido64
Copy link
Member

tido64 commented Mar 8, 2022

Ah… Yeah, I think you're right. I was thinking of the callback, which may return null if there's an error. But for the async API, we should get a rejection, which would throw an exception. Would you mind submitting a PR with the fix? Otherwise, I can try fixing it over the weekend.

mtt87 added a commit to mtt87/async-storage that referenced this issue Mar 9, 2022
@mtt87
Copy link
Contributor Author

mtt87 commented Mar 9, 2022

I created this #767
I did it with the GitHub web UI for a quick change, let's see if all tests pass from CI 😄 otherwise I'll wait for you to do it in the weekend, you are probably more familiar with the codebase 😄

@krizzu
Copy link
Member

krizzu commented Mar 16, 2022

🎉 This issue has been resolved in version 1.16.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants