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

Improve useSubscription hook types #277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aleksandrlat
Copy link

@aleksandrlat aleksandrlat commented Oct 21, 2024

useSubscription allows to omit config.subscription and config.variables if opts.skip is literally true.
I.e. subscription and variables are optional only when we explicitly pass true to skip option:
useSubscription(config, { skip: true }).

But in reality we never do it like this skip: true. We always use some variable. For example like this
useSubscription(config, { skip: !userId }).
But in this case with current typescript type if we use skip: !userId subscription and variables are required.

This PR makes config.subscription and config.variables optional whenever we provide skip option. The value of skip option can be known in runtime only.

The problem code example:

// userId is required variable in subscription
const subscription = graphql`
  subscription UserUpdates(userId: ID!) {
    userUpdates(id: $userId) {
      ...
    }
  }
`

const Component = ({ userId }: { userId?: string }) => {
  const skip = !userId

  useSubscription({ 
    subscription,

    // variables and userId are required here
    // but I don't have userId, so I have to do something like this 
    variables: {
      userId: skip ? '' : userId // or userId: userId || ''
    } 
  }, { skip })
}

With a fix I think code can be clearer:

// userId is required variable in subscription
const subscription = graphql`
  subscription UserUpdates(userId: ID!) {
    userUpdates(id: $userId) {
      ...
    }
  }
`

const Component = ({ userId }: { userId?: string }) => {
  const skip = !userId

  useSubscription({ 
    subscription,

    // we cannot make userId optional, but variables are optional now
    variables: skip ? undefined : {
      userId
    } 
  }, { skip })
}

@morrys I'm not sure what test to write for this, there is no runtime changes. Please let me know if it makes sense at all.
I made a change in our project and there was only 1 typescript error because in that place we didn't use generic param with useSubscription.

Thanks,
Aleksandr

useSubscription allows to omit `config.subscription` and `config.variables` if `opts.skip` is literally true. I.e. `subscription` and `variables` are optional only when we explicitly pass true to skip option: `useSubscription(config, { skip: true })`.

But in reality we never do it like this `skip: true`. We always use some variable like this for example `useSubscription(config, { skip: !userId })`. But in this case with current typescript if we use `skip: !userId` subscription and variables are required.

This change makes `config.subscription` and `config.variables` whenever we provide skip option. The value of skip option can be known in runtime only
@aleksandrlat aleksandrlat marked this pull request as ready for review October 22, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant