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

feat: handle defaultValue not undefined typing #508

Conversation

Ostromecky
Copy link
Contributor

Link to the issue: #506

This is my first contribution to an Open Source project—the issue was labeled as a "Good First Issue."

This implementation aims to allow the compiler to differentiate between the provided arguments, ensuring that the correct code block is returned with the appropriate type. I used function overloading to handle different implementations.

My main concern is the interpretation of defaultValue. While it could be used as fallback data for initialValue, the issue is that we also listen for storage events, which might return an undefined value. I'm also unsure about the naming conventions.

I'm grateful for every remark and comment.

@eneajaho
Copy link
Collaborator

Hello @Ostromecky
Thanks for fixing this and congrats on landing your first contribution 🎉

Regarding your question about listening to storage events that may return undefined.

It would be great if we document this part (in the docs, as it may lead to bugs in the future), and also let's always add undefined in the type if the synchronisation is enabled, this way, the user always will have to check for undefined in that case.

What do you think?

@eneajaho eneajaho added the enhancement New feature or request label Sep 28, 2024
@Ostromecky Ostromecky force-pushed the feature/better-typing-for-injectLocalStorage branch from c2824f8 to aa80de1 Compare September 30, 2024 06:37
@eneajaho
Copy link
Collaborator

eneajaho commented Oct 1, 2024

@all-contributors please add @Ostromecky for code

Copy link
Contributor

@eneajaho

I've put up a pull request to add @Ostromecky! 🎉

@eneajaho eneajaho merged commit dfa4d4e into ngxtension:main Oct 1, 2024
1 check passed
@tanishqmanuja
Copy link

Thanks @Ostromecky 💯

@Ostromecky
Copy link
Contributor Author

Thanks @Ostromecky 💯

It was a pleasure to become a part of something bigger :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants