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

react connectors don't work on react-native #52

Closed
KwanMan opened this issue Aug 20, 2018 · 15 comments
Closed

react connectors don't work on react-native #52

KwanMan opened this issue Aug 20, 2018 · 15 comments
Labels

Comments

@KwanMan
Copy link
Contributor

KwanMan commented Aug 20, 2018

getDerivedStateFromProps behaves differently in react and react-native: facebook/react-native#20759

In react-native, this is only called when props update, not when state updates. This means if you have a state change that does not cause the parent of a connected component to pass down new props, we don't derived the new mapped props.

@KidkArolis
Copy link
Owner

Whaaat, sounds like a bug in react-native? I'll wait for an answer from the react-native team. According the docs this hook should be called before any rerender regardless of reason for render:

"Note that this method is fired on every render, regardless of the cause. "
(https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops)

@KwanMan
Copy link
Contributor Author

KwanMan commented Aug 21, 2018

apparently only a thing since 16.4 facebook/react#12898 but yea i think it's a behaviour that needs to be aligned in react-native

@KidkArolis
Copy link
Owner

@KidkArolis KidkArolis added the bug label Sep 10, 2018
@zyk
Copy link

zyk commented Dec 2, 2018

Hi any updates on this? Have tried downgrading react to 16.3 but it's very hard to to with expo, react-native and react-native-web together in a single project -_- (it works in react-native-web though).

Should I take a look at using hooks instead maybe?

@KidkArolis
Copy link
Owner

I had a quick look at this.

I think the issue is not necessarily anything to do with getDerivedStateFromProps.

To me the issue seems to do with this line detecting DOM:

var canUseDOM = !!(typeof window !== 'undefined' && window.document && window.document.createElement)

If canUseDOM is false, I assumed we're in Server Side Rendering scenario, so react connectors don't subscribe to atom changes. But I didn't account for React Native.

I wonder if there's a way to differentiate between Browser, Server and React Native.

@KwanMan
Copy link
Contributor Author

KwanMan commented Dec 2, 2018

If that's the case, could just have tiny-atom/react-native version of the connectors that always subscribes?

@KidkArolis
Copy link
Owner

Fix released in 3.4.1.

Instead of looking for window.document.createElement, I now just look for navigator which exists in browsers and react-native, but not in node.js. I think this is good enough, because checking for this condition is just a minor, optional optimisation, I'm not even sure we need it. That is, it avoids subscribing to atom when rendering on the server with React.renderToString, but even if you did subscribe, I don't think it would make much of a difference.

@KwanMan
Copy link
Contributor Author

KwanMan commented Dec 3, 2018

If we subscribed on the server, componentWIllUnmount is never called on the server so the observers would never get cleaned up and will add more on each render

@KidkArolis
Copy link
Owner

That's why I added the isServer (or canUseDom previously) check in the first place. But I then quickly realised that you don't reuse the same atom across renders. Because the state could mix up, if you preload user A into atom for request 1, but user B for request 2, you don't want to be reusing atom anyway. This means that after each request/render the atom gets garbage collected.

@zyk
Copy link

zyk commented Dec 3, 2018

Thanks for looking into it!
Sorry to say though, the problem remain. (3.4.2)
I added console.log in getDerivedStateFromProps and render in ConsumerInner.
web: getDerivedStateFromProps + render is called after any state change.
react-native: same as web, but only on initial render.
Any state changes after that only calls render.

@KidkArolis
Copy link
Owner

@zyk interesting, but what do you use getDerivedStateFromProps for? Do your connected components rerender ok? Because to me the react-native example checked into the repo works fine, where you can click increment/decrement multiple times. Are you using the latesrt version of react?

@KidkArolis KidkArolis reopened this Dec 4, 2018
@KidkArolis
Copy link
Owner

@zyk could you paste the snippet of code that's not working for you as expected?

@zyk
Copy link

zyk commented Dec 5, 2018

I put the console logging directly in node_modules/tiny-atom/react.js getDerviedStateFromProps, I did not use getDerived.. myself.

Oh didn't see was a react-native example.
I will check the example and cross-reference the versions against my project, if it doesn't work I'll post a code-snippet.

@zyk
Copy link

zyk commented Dec 5, 2018

Upgrading to latest react worked, thanks!
Awesome project btw!

@KidkArolis
Copy link
Owner

Awesome, glad to hear. Let me know if you run into any other issues. Also, checkout useAtom and useActions react hooks shipping soon in 4.0.0.

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

No branches or pull requests

3 participants