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

feat: add new messages to ui as soon as theyre sent without waiting for subscribtion update #37

Closed
wants to merge 1 commit into from

Conversation

pie6k
Copy link
Collaborator

@pie6k pie6k commented Apr 16, 2021

What changed:

Before: after sending a new message, it was not showing in the UI until the socket subscription picked it. Hasura has 1s socket interval so it often had a visual delay

Now: as soon as mutation resolved, data from mutation is added to the temporary list of 'optimistic messages' to display in the UI .

CleanShot 2021-04-16 at 12 30 16

@pie6k pie6k requested review from koliada and heikir April 16, 2021 10:30
@pie6k pie6k self-assigned this Apr 16, 2021
@linear
Copy link

linear bot commented Apr 16, 2021

@koliada
Copy link
Contributor

koliada commented Apr 16, 2021

By looking at this PR I was feeling there must be a more generic way. And I found this, did you try it?
https://www.apollographql.com/docs/react/performance/optimistic-ui/

@pie6k
Copy link
Collaborator Author

pie6k commented Apr 16, 2021

Yeah, I tried with that and with direct cache access.
apollographql/apollo-kotlin#1722
apollographql/apollo-client#5267

It turned out it works with queries, not subscriptions ¯_(ツ)_/¯

I had some other thoughts how to do it in 'generic way'.

My idea was to use both query and subscribtion at the same time (hasura allows that, you only have to gql.replace(query, subscribtion) and then return 'the newest value' out of both. This way I could update query cache with optimistic update and it would be 'newer value' until next value from subscribtion arrives.

Then I thought to start simple and add some solution like above if more similar use-cases emerge.

But TBH I'm not sure and I'm not fully happy with this PR too.

@koliada
Copy link
Contributor

koliada commented Apr 16, 2021

The fact we need to follow the payload schema with the __typename: "message" property is the biggest red flag here to me. Any other solution which doesn't require manual state composition should be better, IMHO.

@koliada
Copy link
Contributor

koliada commented Apr 16, 2021

Basically, the solution presented in the PR is not sustainable enough.

Copy link
Contributor

@heikir heikir left a comment

Choose a reason for hiding this comment

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

Looks good overall.

@heikir
Copy link
Contributor

heikir commented Apr 17, 2021

I agree with @koliada . The PR looks ok, but implementing the other way solution and making it generic so that it could be applied in the future in other components probably makes more sense.

@@ -43,7 +43,8 @@
"styled-normalize": "^8.0.7",
"styled-reset": "^4.3.4",
"subscriptions-transport-ws": "^0.9.18",
"react-hook-form": "^7.0.6"
"react-hook-form": "^7.0.6",
"immer": "^9.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to sort the manually added dependencies for better visibility :)

@heikir
Copy link
Contributor

heikir commented Apr 21, 2021

@pie6k what's your take on this? Would you like to re-implement this or leave it be for now? I don't have a strong opinion on this.

@pie6k
Copy link
Collaborator Author

pie6k commented Apr 21, 2021

I think I'll re-implement and use this opportunity to organize graphql definitions and hooks in a bit different way (we've been discussing it a bit recently, but I can elaborate)

@heikir
Copy link
Contributor

heikir commented Apr 21, 2021

Sounds good. Writing it down, what you would like to change, would help me to participate in the discussion.

@pie6k
Copy link
Collaborator Author

pie6k commented Apr 21, 2021

Fair, added context to https://meetnomore.com/room/ptTTTfy12hdK3mv347R5 > graphql agenda point (:< not able to get link to agenda point)

@pie6k
Copy link
Collaborator Author

pie6k commented Apr 22, 2021

I'll redo this work with different graphql approach

@pie6k pie6k closed this Apr 22, 2021
@koliada koliada deleted the aca-274-messages-subscribtion-should-instantly branch May 18, 2021 14:38
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.

3 participants