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

[wip] shift to optimistic updates #122

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

Conversation

avimoondra
Copy link
Owner

No description provided.

Comment on lines +168 to +175
React.useEffect(() => {
return subscribeToMore({
variables: {
joinCode: props.joinCode,
},
document: CurrentGameSubDocument,
})
}, [data?.games[0]])
Copy link
Owner Author

Choose a reason for hiding this comment

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

subscribe this way, then can use optimistic response instead of local states.

related to: apollographql/apollo-client#5267

Copy link
Owner Author

@avimoondra avimoondra Dec 5, 2020

Choose a reason for hiding this comment

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

some interesting behavior here - some flashes on refresh, and two web sockets get opened.

starting to think sub might be overkill for this whole thing, we could prob do with 1 sec polling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I ran into this same thing recently and landed on the same GitHub issue / solution; it worked well.

You're resubscribing a bunch in the current state and missing an updateQuery which might help:

Suggested change
React.useEffect(() => {
return subscribeToMore({
variables: {
joinCode: props.joinCode,
},
document: CurrentGameSubDocument,
})
}, [data?.games[0]])
React.useEffect(() => {
return subscribeToMore({
variables: {
joinCode: props.joinCode,
},
document: CurrentGameSubDocument,
updateQuery: (data, { subscriptionData }) => {
return subscriptionData.data || data
},
})
}, [props.joinCode])

The subscription and query are the same in this context, so updateQuery is dead simple, but it gives you the flexibility to deviate from the query (i.e. only make some things realtime, subscribe to a handful of smaller subscriptions, etc).

Copy link
Owner Author

@avimoondra avimoondra Dec 5, 2020

Choose a reason for hiding this comment

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

Yea - I was meaning to check if updateQuery defaults to exactly that if not passed in - but will add it explicitly to see the effect.

I also thought direction of flexibility was 👍 e.g... we are subscribing for everything once game is started. But practically... the most settings, players and cards cannot change after the game has started, so based on the game state we could actually run different subscriptions (so long as we close out the previous gracefully).

Copy link
Collaborator

@namoscato namoscato Dec 5, 2020

Choose a reason for hiding this comment

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

I was meaning to check if updateQuery defaults to exactly that if not passed in

It seems like it doesn't on glance, but I'm not sure how it would have been working without it now if that's the case 🤔

based on the game state we could actually run different subscriptions

Yeah, love that.

Comment on lines +44 to +50
// const [cardPlayStyle, setCardPlayStyle] = React.useState(
// GameCardPlayStyleEnum.PlayersSubmitWords
// )

React.useEffect(() => {
setCardPlayStyle(currentGame.card_play_style)
}, [currentGame.card_play_style])
// React.useEffect(() => {
// setCardPlayStyle(currentGame.card_play_style)
// }, [currentGame.card_play_style])
Copy link
Owner Author

Choose a reason for hiding this comment

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

stop using local state

Comment on lines +168 to +175
React.useEffect(() => {
return subscribeToMore({
variables: {
joinCode: props.joinCode,
},
document: CurrentGameSubDocument,
})
}, [data?.games[0]])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I ran into this same thing recently and landed on the same GitHub issue / solution; it worked well.

You're resubscribing a bunch in the current state and missing an updateQuery which might help:

Suggested change
React.useEffect(() => {
return subscribeToMore({
variables: {
joinCode: props.joinCode,
},
document: CurrentGameSubDocument,
})
}, [data?.games[0]])
React.useEffect(() => {
return subscribeToMore({
variables: {
joinCode: props.joinCode,
},
document: CurrentGameSubDocument,
updateQuery: (data, { subscriptionData }) => {
return subscriptionData.data || data
},
})
}, [props.joinCode])

The subscription and query are the same in this context, so updateQuery is dead simple, but it gives you the flexibility to deviate from the query (i.e. only make some things realtime, subscribe to a handful of smaller subscriptions, etc).

updateGameSettings({
variables: {
id: currentGame.id,
input: {
card_play_style: value as GameCardPlayStyleEnum,
},
},
optimisticResponse: {
update_games_by_pk: {
...currentGame,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if two fragments would help / minimize the scope of these setting cache updates, i.e.

  1. fragment GameSettings on games with the (base) UpdateGameSettings properties

  2. a full fragment that "extends" from that one:

    fragment Game on games {
        ...GameSettings
        state
        (etc)

    with the rest of the CurrentGame properties

Copy link
Owner Author

Choose a reason for hiding this comment

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

Most definitely 👍 - there's no use of fragments across this whole codebase yet! Bout damn time.

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.

2 participants