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

docs: update subscribeToMore for getDerivedStateFromProps #3680

Merged
merged 5 commits into from
Aug 17, 2018
Merged

docs: update subscribeToMore for getDerivedStateFromProps #3680

merged 5 commits into from
Aug 17, 2018

Conversation

brainkim
Copy link
Contributor

@brainkim brainkim commented Jul 12, 2018

  • docs

@apollo-cla
Copy link

@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/

@ghost ghost added the 📝 documentation label Jul 12, 2018
@brainkim
Copy link
Contributor Author

@ghost ghost added the 📝 documentation label Jul 12, 2018
@brainkim
Copy link
Contributor Author

brainkim commented Jul 20, 2018

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

@hwillson
Copy link
Member

hwillson commented Jul 30, 2018

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!

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@brainkim
Copy link
Contributor Author

brainkim commented Jul 31, 2018

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
Copy link
Contributor Author

brainkim commented Aug 8, 2018

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
Copy link
Member

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!

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hwillson hwillson merged commit 405a8f6 into apollographql:master Aug 17, 2018
@brainkim
Copy link
Contributor Author

Thank you for all the work you do on Apollo!

@yuxiaoma
Copy link

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
Copy link
Contributor Author

brainkim commented Sep 4, 2018

@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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants