docs: update subscribeToMore for getDerivedStateFromProps

[brainkim]

• docs

[apollo-cla]

@brainkim: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[brainkim]

apollographql/react-apollo#2181

[brainkim]

I can force-push rebase whenever you want me too. @hwillson

[hwillson]

Hi @brainkim - first off, thanks for working on this!

I'm not a huge fan of copying props into state, whenever it can be avoided. In this case, instead of using getDerivedStateFromProps, how about something more along the lines of:

class SubscriptionComponent extends Component {
  constructor(props) {
    super(props);
    this.unsubscribe = null;
  }

  componentDidMount() {
    if (!this.props.loading) {
      this.initSubscription();
    }
  }

  componentDidUpdate(prevProps) {
    if (!this.props.loading) {
      if (this.unsubscribe && 
        (prevProps.subscriptionParam !== this.props.subscriptionParam)
      ) {
        this.unsubscribe();
        this.unsubscribe = null;
      }
      this.initSubscription();
    }    
  }

  componentWillUnmount() {
    if (this.unsubscribe) {
      this.unsubscribe();
      this.unsubscribe = null;
    }
  }

  initSubscription() {
    if (!this.unsubscribe) {
      this.unsubscribe = this.props.data.subscribeToMore({
        document: gql`subscription {...}`,
        updateQuery: (previousResult, { subscriptionData, variables }) => {
          // Perform updates on previousResult with subscriptionData
          return updatedResult;
        }
      });  
    }
  }

  render() {
    ...
  }
}

That should help tick all of the boxes the previous example was handling, and get us away from copying props into state. If you're interested in updating this PR to follow something more along these lines, that would be awesome!

[hwillson]

See my comments in #3680 (comment) - thanks!

[brainkim]

Hmmm. I agree that duplicating props in state is worrying but I think it might be necessary here b/c of edge cases relating to props.loading and stale subscriptions. The code above is assuming a change in the subscriptionParam will force the component to go into a loading state, but I don't think there's anything about apollo graphql or graphql subscriptions that require a component to refetch on a subscription param change, insofar as the queries used to fetch and subscribe are separate and do not necessarily use the same variables (although most of the time they do). If we change the subscription param of the component above in one update and change loading in another, the subscription will go out of sync with the current query.

TBH I don't have a 100% understanding of the correct use cases for getDerivedStateFromProps, but I do think my example more robust, and it's already been used (and seems bug-free) in a project I'm working on.

If you disagree, feel free to close the PR and merge a different one!
Brian

[brainkim]

Pinging @hwillson any thoughts? I checked your code and it doesn’t work even in the happy path case b/c by the time this.props.loading is false prevProps.subscriptionParam and this.props.subscriptionParam will have the same value, making the subscription stale. Like I said, cuz this is a documentation change, I don’t mind if you close in favor of a different fix.

[hwillson]

Thanks for the feedback @brainkim - I'll take another look at this shortly. For now let's leave this open though. I agree with you that this should be covered in the docs, so we'll get something in place shortly. Thanks for your patience!

[hwillson]

Let's do it @brainkim - thanks very much for working on this!

[brainkim]

Thank you for all the work you do on Apollo!

[yuxiaoma]

I have a few questions about this update:

1. From my understanding, getDerivedStateFromProps is meant to be pure, free from any side-effects. “getDerivedStateFromProps is invoked right before calling the render method, both on the initial mount and on subsequent updates. It should return an object to update the state, or null to update nothing. This method exists for rare use cases where the state depends on changes in props over time.” Also, it is a static function may be reused by another component. Should we be really putting something like subscribeToMore in there?

2. In my project, I am putting subscribeToMore inside componentDidMount, and there is also the suggestion of putting it in componentWillMount. Both worked with my project, is there any issue with my approach I should look out for?

[brainkim]

@yuxiaoma

1. I don’t see any references to getDerivedStateFromProps having to be pure in the official docs, but insofar as the code in this PR does perform side-effects, they’re limited to calling the current subscription’s unsubscribe method which is stored in state and I don’t see how that might become a problem.

2. using componentDidMount is fine, but if your component expects a new subscription with different subscribe query parameters, then your component is going to become stale when the props change accordingly. componentWillMount was deprecated last I checked.