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

fix: initial data load in useSubscriptionGraphql #10925

Closed
wants to merge 1 commit into from

Conversation

Jarsen136
Copy link
Contributor

Thank you for your contribution to the Koda - Generative Art Marketplace.

👇 __ Let's make a quick check before the contribution.

PR Type

  • Bugfix

Needs QA check

  • @kodadot/qa-guild please review

Context

For the composable useSubscriptionGraphql, the initial data fetching should not trigger onChange callback

@Jarsen136 Jarsen136 requested a review from a team as a code owner September 1, 2024 21:34
@Jarsen136 Jarsen136 requested review from preschian and hassnian and removed request for a team September 1, 2024 21:34
Copy link

netlify bot commented Sep 1, 2024

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 30eca67
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/66d4dded6113d2000851ad3b
😎 Deploy Preview https://deploy-preview-10925--koda-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

sonarqubecloud bot commented Sep 1, 2024

Comment on lines +47 to +50
if (lastQueryResult !== null) {
$consola.log(`[Graphql Subscription] New changes: ${JSON.stringify(newResult)}`)
onChange({ data: newResult })
}
Copy link
Contributor

@hassnian hassnian Sep 2, 2024

Choose a reason for hiding this comment

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

this is not how the old implementation of useSubscriptionGraphql.ts worked

onChange was triggered when fetching also for the first time

seeBuy.vue how it's clear that it depends on the first fetch to also trigger onChange, with this change the buy modal never loads

if for some reason you want this behaviour i would make it optional as. param, when needed, otherwise you might be breaking other code

on a similar note atm the polling waits 6 seconds before making the first fetch, I'd just fix that on this pr

CleanShot 2024-09-02 at 07 01 17@2x

from #10911, pls suggest if you have any better fix :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, originally it was supposed to be used to monitor upcoming changes. Since there is a need to trigger the first request, let's keep it as it is.
Thanks for your explanation.

@Jarsen136 Jarsen136 closed this Sep 2, 2024
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