search

nanostores / query

⚡️ Powerful data fetching library for Nano Stores. TS/JS. Framework agnostic.
MIT License
228 stars 10 forks source link

fix: support react native #41

Closed hemandev closed 6 months ago

hemandev commented 6 months ago

query is failing when it is used inside react-native. The reason is because it is trying to execute addEventListener, which doesn't exist in React Native. We are already checking typeof window === 'undefined for Server, we just need to add one more extra check to see if addEventListener exist in window

IMG_0047

hemandev commented 6 months ago

@dkzlv Would you mind taking a look at this? Its a blocker for us on using it in React Native. Thanks!

dkzlv commented 6 months ago

@hemandev Thanks for the PR.

I'm not particularly against this implementation, but can you please confirm if this solution works as well? I feel like it's more specific. TLDR: navigator.product !== 'ReactNative'. If it works (or any combination of platform and product in navigator), I'd prefer this solution over yours.

Also, I'll merge it, but also I'll make some changes to overall architecture in a separate PR. I feel like this needs a bit more documentation and predictability: we already have 3 environments, and each one may have its own "interface". So, while RN doesn't have blur/focus events on document, they definitely have their own events for the same thing. It feels like if we want to support RN, we need to write code for these events as well, otherwise RN implementation doesn't support the same features as Web version.

UPD: those events are supported, as I suspected, just need to use other object to subscribe.

hemandev commented 6 months ago

@hemandev Thanks for the PR.

I'm not particularly against this implementation, but can you please confirm if this solution works as well? I feel like it's more specific. TLDR: navigator.product !== 'ReactNative'. If it works (or any combination of platform and product in navigator), I'd prefer this solution over yours.

Also, I'll merge it, but also I'll make some changes to overall architecture in a separate PR. I feel like this needs a bit more documentation and predictability: we already have 3 environments, and each one may have its own "interface". So, while RN doesn't have blur/focus events on document, they definitely have their own events for the same thing. It feels like if we want to support RN, we need to write code for these events as well, otherwise RN implementation doesn't support the same features as Web version.

UPD: those events are supported, as I suspected, just need to use other object to subscribe.

Just tried it out and its working. I was also thinking of doing the same thing, but I thought of reducing the surface area for the context. Also navigator.product is deprecated in web. But yeah, it makes sense for ReactNative.

Also, I'll merge it, but also I'll make some changes to overall architecture in a separate PR. I feel like this needs a bit more documentation and predictability

That would be great. We are going all in with this library and its been great so far.

dkzlv commented 6 months ago

I'll release it today, probably, along with some unrelated changes. Thanks!

hemandev commented 6 months ago

I'll release it today, probably, along with some unrelated changes. Thanks!

Awesome 🎉 . Thanks as always