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.

Sign up for GitHub

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
Open

[wip] shift to optimistic updates #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions app/src/components/GameRoutes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@ import {
} from "contexts/CurrentPlayer"
import { NotificationContext } from "contexts/Notification"
import {
CurrentGameSubDocument,
CurrentPlayerDocument,
CurrentPlayerQuery,
GameByJoinCodeDocument,
useCurrentGameSubscription,
useCurrentGameQuery,
useJoinGameMutation,
} from "generated/graphql"
import CardSubmission from "pages/CardSubmission"
Expand Down Expand Up @@ -158,12 +159,21 @@ function CurrentGameProvider(props: {
joinCode: string
children: React.ReactNode
}) {
const { data, loading } = useCurrentGameSubscription({
const { data, loading, subscribeToMore } = useCurrentGameQuery({
variables: {
joinCode: props.joinCode,
},
})

React.useEffect(() => {
return subscribeToMore({
variables: {
joinCode: props.joinCode,
},
document: CurrentGameSubDocument,
})
}, [data?.games[0]])
Comment on lines +168 to +175
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.


if (!loading && !data?.games[0]) {
return <Redirect to={routes.root}></Redirect>
}
Expand Down
6 changes: 2 additions & 4 deletions app/src/components/ScreenCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,13 @@ import BowlCard from "components/BowlCard"
import PlayerChip from "components/PlayerChip"
import { CurrentGameContext } from "contexts/CurrentGame"
import {
CurrentGameSubscription,
CurrentGameQuery,
useAcceptCardMutation,
useRejectCardMutation,
} from "generated/graphql"
import * as React from "react"

function ScreenCard(props: {
card: CurrentGameSubscription["games"][0]["cards"][0]
}) {
function ScreenCard(props: { card: CurrentGameQuery["games"][0]["cards"][0] }) {
const currentGame = React.useContext(CurrentGameContext)
const [acceptCard] = useAcceptCardMutation()
const [rejectCard] = useRejectCardMutation()
Expand Down
4 changes: 2 additions & 2 deletions app/src/contexts/CurrentGame.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import {
CurrentGameSubscription,
CurrentGameQuery,
GameCardPlayStyleEnum,
GameStateEnum,
} from "generated/graphql"
import { createContext } from "react"

export type CurrentGameContextType = CurrentGameSubscription["games"][0]
export type CurrentGameContextType = CurrentGameQuery["games"][0]

export const CurrentGameContext = createContext<CurrentGameContextType>({
id: "",
Expand Down
48 changes: 47 additions & 1 deletion app/src/graphql/operations.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,53 @@ query CurrentPlayer($clientUuid: uuid!, $joinCode: String!) {
}
}

subscription CurrentGame($joinCode: String!) {
query CurrentGame($joinCode: String!) {
games(where: { join_code: { _eq: $joinCode } }) {
id
join_code
starting_letter
seconds_per_turn
num_entries_per_player
allow_card_skips
screen_cards
card_play_style
state
host {
id
username
}
rounds(order_by: { order_sequence: asc }) {
id
value
}
cards(
where: {
_or: [{ is_allowed: { _is_null: true } }, { is_allowed: { _eq: true } }]
}
) {
id
word
player_id
is_allowed
}
players {
id
client_uuid
username
team
team_sequence
}
turns(order_by: { sequential_id: asc }) {
id
player_id
started_at
completed_card_ids
seconds_per_turn_override
}
}
}

subscription CurrentGameSub($joinCode: String!) {
games(where: { join_code: { _eq: $joinCode } }) {
id
join_code
Expand Down
6 changes: 3 additions & 3 deletions app/src/lib/score.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { CurrentGameSubscription } from "generated/graphql"
import { CurrentGameQuery } from "generated/graphql"
import { Team } from "lib/team"
import { filter, flatMap } from "lodash"

export function teamScore(
team: Team,
turns: CurrentGameSubscription["games"][0]["turns"],
players: CurrentGameSubscription["games"][0]["players"]
turns: CurrentGameQuery["games"][0]["turns"],
players: CurrentGameQuery["games"][0]["players"]
) {
const teamPlayerIds = filter(players, (player) => player.team === team).map(
(player) => player.id
Expand Down
4 changes: 2 additions & 2 deletions app/src/lib/team.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { CurrentGameSubscription } from "generated/graphql"
import { CurrentGameQuery } from "generated/graphql"
import { cloneDeep, filter, find, remove, shuffle } from "lodash"

export enum Team {
Expand All @@ -25,7 +25,7 @@ function addTeamAndSequence(players: Players, team: Team) {
// > [1, 2, 3, 4] (first half will always be equal or 1 longer)
// [1,2,3,4,5,6,7].splice(Math.ceil(7/ 2), 7)
// > [5, 6, 7]
type Player = CurrentGameSubscription["games"][0]["players"][0]
type Player = CurrentGameQuery["games"][0]["players"][0]
type Players = Array<Player>
export function teamsWithSequence(players: Players) {
const shuffledPlayers = shuffle(players)
Expand Down
22 changes: 10 additions & 12 deletions app/src/lib/turn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { CurrentGameSubscription } from "generated/graphql"
import { CurrentGameQuery } from "generated/graphql"
import { Team } from "lib/team"
import {
countBy,
Expand All @@ -20,8 +20,8 @@ export enum ActiveTurnPlayState {
}

export function nextPlayerForSameTeam(
activePlayer: CurrentGameSubscription["games"][0]["players"][0],
players: CurrentGameSubscription["games"][0]["players"]
activePlayer: CurrentGameQuery["games"][0]["players"][0],
players: CurrentGameQuery["games"][0]["players"]
) {
const sameTeamPlayers = filter(
players,
Expand All @@ -37,9 +37,9 @@ export function nextPlayerForSameTeam(
}

export function nextPlayerForNextTeam(
activePlayer: CurrentGameSubscription["games"][0]["players"][0] | null,
turns: CurrentGameSubscription["games"][0]["turns"],
players: CurrentGameSubscription["games"][0]["players"]
activePlayer: CurrentGameQuery["games"][0]["players"][0] | null,
turns: CurrentGameQuery["games"][0]["turns"],
players: CurrentGameQuery["games"][0]["players"]
) {
if (!activePlayer) {
return sortBy(players, ["team_sequence"])[0]
Expand Down Expand Up @@ -72,15 +72,13 @@ export function nextPlayerForNextTeam(
return nextPlayerFromNextTeamToPlay
}

export function completedCardIds(
turns: CurrentGameSubscription["games"][0]["turns"]
) {
export function completedCardIds(turns: CurrentGameQuery["games"][0]["turns"]) {
return flatMap(turns, (turn) => turn.completed_card_ids)
}

export function drawableCards(
turns: CurrentGameSubscription["games"][0]["turns"],
cards: CurrentGameSubscription["games"][0]["cards"]
turns: CurrentGameQuery["games"][0]["turns"],
cards: CurrentGameQuery["games"][0]["cards"]
) {
const allCompletedCardIds = flatMap(turns, (turn) => turn.completed_card_ids)

Expand All @@ -104,7 +102,7 @@ export function drawableCards(
}

export function drawableCardsWithoutCompletedCardsInActiveTurn(
cards: CurrentGameSubscription["games"][0]["cards"],
cards: CurrentGameQuery["games"][0]["cards"],
completedCardIdsInActiveTurn: Array<number>
) {
return reject(cards, (card) => completedCardIdsInActiveTurn.includes(card.id))
Expand Down
28 changes: 18 additions & 10 deletions app/src/pages/Lobby/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ function Lobby() {
const location = useLocation()
const [hideShare, setHideShare] = React.useState(false)
const [updateGameSettings] = useUpdateGameSettingsMutation()
const [cardPlayStyle, setCardPlayStyle] = React.useState(
GameCardPlayStyleEnum.PlayersSubmitWords
)
// 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])
Comment on lines +44 to +50
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


const [wordList, setWordList] = React.useState("")
const debouncedSetWordList = React.useRef(
Expand Down Expand Up @@ -94,16 +94,22 @@ function Lobby() {
<>
<div className={classes.section}>
<SettingsSection
cardPlayStyle={cardPlayStyle}
cardPlayStyle={currentGame.card_play_style}
setCardPlayStyle={(value) => {
setCardPlayStyle(value as GameCardPlayStyleEnum)
// setCardPlayStyle(value as GameCardPlayStyleEnum)
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.

card_play_style: value as GameCardPlayStyleEnum,
},
},
})
}}
debouncedSetWordList={debouncedSetWordList}
Expand All @@ -129,7 +135,7 @@ function Lobby() {
</Grid>
<WaitingRoom
wordList={wordList}
cardPlayStyle={cardPlayStyle}
cardPlayStyle={currentGame.card_play_style}
></WaitingRoom>
</Grid>
</Grid>
Expand All @@ -139,7 +145,9 @@ function Lobby() {
<>
<Divider variant="middle"></Divider>
<div className={classes.section}>
<SettingsSection cardPlayStyle={cardPlayStyle}></SettingsSection>
<SettingsSection
cardPlayStyle={currentGame.card_play_style}
></SettingsSection>
</div>
</>
)}
Expand Down
6 changes: 3 additions & 3 deletions app/src/pages/Play/HostControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { grey } from "@material-ui/core/colors"
import PlayerChip from "components/PlayerChip"
import { CurrentGameContext } from "contexts/CurrentGame"
import {
CurrentGameSubscription,
CurrentGameQuery,
useEndCurrentTurnAndStartNextTurnMutation,
} from "generated/graphql"
import { useTitleStyle } from "index"
Expand All @@ -12,8 +12,8 @@ import { nextPlayerForNextTeam, nextPlayerForSameTeam } from "lib/turn"
import * as React from "react"

function HostControls(props: {
activeTurn: CurrentGameSubscription["games"][0]["turns"][0]
activePlayer: CurrentGameSubscription["games"][0]["players"][0]
activeTurn: CurrentGameQuery["games"][0]["turns"][0]
activePlayer: CurrentGameQuery["games"][0]["players"][0]
currentRoundId: number
}) {
const currentGame = React.useContext(CurrentGameContext)
Expand Down
6 changes: 3 additions & 3 deletions app/src/pages/Play/TeamContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ import { Box, Grid } from "@material-ui/core"
import PlayerChip from "components/PlayerChip"
import { CurrentGameContext } from "contexts/CurrentGame"
import { CurrentPlayerContext } from "contexts/CurrentPlayer"
import { CurrentGameSubscription } from "generated/graphql"
import { CurrentGameQuery } from "generated/graphql"
import { nextPlayerForNextTeam } from "lib/turn"
import * as React from "react"

type Props = {
activePlayer: CurrentGameSubscription["games"][0]["players"][0]
activeTurn: CurrentGameSubscription["games"][0]["turns"][0]
activePlayer: CurrentGameQuery["games"][0]["players"][0]
activeTurn: CurrentGameQuery["games"][0]["turns"][0]
}

export function YourTeamTurnContent(props: Props) {
Expand Down
12 changes: 6 additions & 6 deletions app/src/pages/Play/YourTurnContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import BowlCard from "components/BowlCard"
import PlayerChip from "components/PlayerChip"
import { CurrentGameContext } from "contexts/CurrentGame"
import {
CurrentGameSubscription,
CurrentGameQuery,
Rounds,
useEndCurrentTurnAndStartNextTurnMutation,
useStartTurnMutation,
Expand Down Expand Up @@ -47,10 +47,10 @@ const GreenCheckbox = withStyles({
})((props: CheckboxProps) => <Checkbox color="default" {...props} />)

function YourTurnContent(props: {
yourTeamPlayers: CurrentGameSubscription["games"][0]["players"]
cardsInBowl: CurrentGameSubscription["games"][0]["cards"]
activePlayer: CurrentGameSubscription["games"][0]["players"][0]
activeTurn: CurrentGameSubscription["games"][0]["turns"][0]
yourTeamPlayers: CurrentGameQuery["games"][0]["players"]
cardsInBowl: CurrentGameQuery["games"][0]["cards"]
activePlayer: CurrentGameQuery["games"][0]["players"][0]
activeTurn: CurrentGameQuery["games"][0]["turns"][0]
activeTurnPlayState: ActiveTurnPlayState
secondsLeft: number
currentRoundId: Rounds["id"]
Expand All @@ -67,7 +67,7 @@ function YourTurnContent(props: {
const [skippingTurn, setSkippingTurn] = React.useState(false)

const [activeCard, setActiveCard] = React.useState<
CurrentGameSubscription["games"][0]["cards"][0] | null
CurrentGameQuery["games"][0]["cards"][0] | null
>(null)

const [shownCardsInActiveTurn, setShownCardsInActiveTurn] = React.useState<
Expand Down