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

added polling when checking if a user is donating to a collective #137

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

krisbitney
Copy link
Contributor

This should improve the speed at which the app can detect if it should show the option to stop donating

@L03TJ3
Copy link
Collaborator

L03TJ3 commented Jan 31, 2024

@krisbitney if this only for the case where a user just started a stream donation its too extensive.

  1. it increases the amount of network requests by a factor of 6 (25 sec: 380 network requests / now: 59 requests)
  2. do we really need to keep pulling data from subgraph every x seconds? @sirpy

@L03TJ3 L03TJ3 requested a review from sirpy January 31, 2024 09:28
@krisbitney
Copy link
Contributor Author

krisbitney commented Jan 31, 2024

@krisbitney if this only for the case where a user just started a stream donation its too extensive.

  1. it increases the amount of network requests by a factor of 6 (25 sec: 380 network requests / now: 59 requests)
  2. do we really need to keep pulling data from subgraph every x seconds? @sirpy

Hmm it should check for new data every 1 second, and only for this query on this page. Are you sure it is increasing the amount of network requests that significantly?

It shouldn't pull data from the subgraph unless there is new data.

The purpose was to resolve this issue: #136

@L03TJ3
Copy link
Collaborator

L03TJ3 commented Jan 31, 2024

@krisbitney compared it against dev-goodcollective.vercel.app. below is from this pr deployment
Screenshot from 2024-01-31 17-25-03

@L03TJ3
Copy link
Collaborator

L03TJ3 commented Jan 31, 2024

Can this be because of the flowrate and timestamps updating?
it keeps fetching data every 5 seconds by default (before this fix)

@krisbitney
Copy link
Contributor Author

krisbitney commented Jan 31, 2024

Can this be because of the flowrate and timestamps updating? it keeps fetching data every 5 seconds by default (before this fix)

The flowrate doesn't work by repeatedly fetching. It shouldn't affect it. I'm not sure the cause but I will look into it.

It might just be because of fetching every 1 second vs every 5 seconds. I guess that would be a 5x increase, but it should be only for this one query.

@L03TJ3
Copy link
Collaborator

L03TJ3 commented Jan 31, 2024

No I meant because of time stamps updating there is always new data to fetch?

@krisbitney
Copy link
Contributor Author

No I meant because of time stamps updating there is always new data to fetch?

If you mean this timestamp, it only updates when the smart contract event is triggered. The timestamp refers to that last time the donor-collective relationship was updated. It shouldn't change otherwise.

  query DONOR_COLLECTIVE_BY_ENTITIES($donor: String, $collective: String) {
    donorCollectives(where: { donor: $donor, collective: $collective }) {
      id
      donor {
        id
      }
      collective {
        id
      }
      contribution
      flowRate
      timestamp
    }
  }

@krisbitney
Copy link
Contributor Author

krisbitney commented Jan 31, 2024

@krisbitney if this only for the case where a user just started a stream donation its too extensive.

  1. it increases the amount of network requests by a factor of 6 (25 sec: 380 network requests / now: 59 requests)
  2. do we really need to keep pulling data from subgraph every x seconds? @sirpy

@L03TJ3 In your screenshot, you got 46 API requests related to the graph. I got a similar number in my test locally.

When testing the master branch locally, I got about 30.

Here are screenshots. The first one is the master branch, and the second is this PR's branch.

Screenshot 2024-01-31 at 6 54 42 PM Screenshot 2024-01-31 at 6 56 24 PM

@L03TJ3
Copy link
Collaborator

L03TJ3 commented Jan 31, 2024

Ye I stupidely looked at the wrong digita, apologies

@L03TJ3
Copy link
Collaborator

L03TJ3 commented Feb 1, 2024

@krisbitney but besides my wrong analysis about the count of the events. you are saying that it should not keep polling indefinitely correct? so is there then still something wrong with the caching, or?? idk

@krisbitney
Copy link
Contributor Author

@krisbitney but besides my wrong analysis about the count of the events. you are saying that it should not keep polling indefinitely correct? so is there then still something wrong with the caching, or?? idk

It polls indefinitely, so long as the user is on this page. There is nothing wrong with the caching. The polling is intended to make sure the data this query is fetching gets updates as soon as possible. It checks if there is new data, and only downloads it if it exists.

@sirpy
Copy link
Contributor

sirpy commented Feb 1, 2024

@krisbitney @L03TJ3
if we use polling, lets put this as an env variable.
1 second is too much. should be something like 30 seconds. its not that critical to immediately register the user tx

the optimal solution from my POV, is to refetch just once X seconds after the user TX completes

@krisbitney
Copy link
Contributor Author

@krisbitney @L03TJ3 if we use polling, lets put this as an env variable. 1 second is too much. should be something like 30 seconds. its not that critical to immediately register the user tx

the optimal solution from my POV, is to refetch just once X seconds after the user TX completes

I changed it to an env variable with a default 30 seconds. If it doesn't fix #136 then you can change the interval or implement Hadar's solution.

@L03TJ3 L03TJ3 merged commit 1435336 into master Feb 1, 2024
2 checks passed
@L03TJ3 L03TJ3 deleted the kris/poll-is-donating branch February 1, 2024 11:52
@decentralauren
Copy link

Was better and will work for now. @patpedrosa @L03TJ3 Created a ticket to revisit modal language to message that things may take a while / encourage patience :)

Closing this issue.

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.

4 participants