IjzerenHein / firestorter

Use Google Firestore in React with zero effort, using MobX 🤘
http://firestorter.com
MIT License
378 stars 50 forks source link

unsafe lifecycle methods #15

Closed cworf closed 6 years ago

cworf commented 6 years ago

hey, a while back you gave me some advice on how to fix my problem getting subcollection paths from prop using the following method...

const EventClient = observer(class EventClient extends Component {

  eventClientDoc = new Document(this.props.client); // 

  componentWillReceiveProps(newProps) {
    if (newProps.client !== this.props.client) {
      this.eventClientDoc.path = this.props.client;
    }
  }

  render(){\
...

can you perhaps advise on how best to fix this method for the new safe static method getDerivedStateFromProps() which is to replace componentWillReceiveProps() in React v.17?

ive been at it for at least a few hours, and I cant seem to get anywhere close, the issue is that getDerivedStateFromProps() is static and can not access this so I have to declare eventClientDoc in state. Using the return of getDerivedStateFromProps() to update this.state.eventClientDoc to a new Document does in fact seem up update the path of the document to the correct path, however no matter what I do I can not get things to render without errors, even using

render(){
  if (!this.state.eventClientDoc.data) return null
  ...

are you familiar with this new method? any ideas?

IjzerenHein commented 6 years ago

Hi Colin. So what I would probably do, is to move all the code to getDerivedStateFromProps like this:

const EventClient = observer(class EventClient extends Component {

  static getDerivedStateFromProps(nextProps, prevState) {
    const doc = prevState.doc || new Document();
    if (doc.path !== nextProps.client) doc.path = nextProps.client;
    return {
      doc
    };
  }

  render() {
    console.log(this.state.doc.data);
    ...
  }
});

This should handle the lifecycle and path update of the Document correctly. I haven't tested this though, I will when I have a bit more time. Let me know whether that solves it.

cworf commented 6 years ago

Finally got around to testing this out, and it worked! I was definitely overthinking this. what makes this solution even better than the old componentWillReceiveProps() method which doesn't fire on first mount and only fires on successive mounts, as opposed to getDerivedStateFromProps() which fires on the first mount is that you no longer need to check for a snapshot using

  render() {
    const {data, snapshot} = this.state.eventClientDoc
    if (!snapshot) return null;
    console.log(data);

not super exciting, but still a bit cleaner overall. I can definitely see why this lifecycle method would be safer than its counterpart.

Thanks for your help!

cworf commented 6 years ago

oh, also for anyone else reading this thread, you need to define state first or it will yell at you. :)

IjzerenHein commented 6 years ago

Alright, great to hear that helped!

And are you really sure you don't need to check for the snapshot? Cause I can imagine that it still doesn't have data yet on (first) render.

oh, also for anyone else reading this thread, you need to define state first or it will yell at you. :)

And what do you mean by define state otherwise it will yell at you? 🤔

cworf commented 6 years ago

Im sorry I lied about the snapshot thing. it must have been working for me via caching or something, but after refreshing again after a few minutes, I got an undefined error.

And what do you mean by define state otherwise it will yell at you?

I mean you will get an error if you dont define the document in state as null like so

state = {
  doc: null
}