-
Notifications
You must be signed in to change notification settings - Fork 0
DEMOS-1043: Remove debug statements and fix refetchQueries #546
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
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
|
||
| const VIRUS_SCAN_MAX_ATTEMPTS = 10; | ||
| const DOCUMENT_POLL_INTERVAL_MS = 1_000; | ||
| const DOCUMENT_POLL_INTERVAL_MS = 2_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do half the requests, doesn't seem like this usually finishes within a second but might finish within 2
| await clickPromise; | ||
| // Advance timer to allow polling to complete | ||
| await vi.advanceTimersByTimeAsync(1000); | ||
| await vi.advanceTimersByTimeAsync(DOCUMENT_POLL_INTERVAL_MS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests now reference the real values in case those change the tests will still be accurate
connor-parke-icf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks fine to me! i think there may be a cleaner path to updating the frontend data from polling, but im not familiar enough with how polling plays with how the Apollo client stores its query results to push in any particular direction
| if (refetchQueries) { | ||
| await client.refetchQueries({ include: refetchQueries }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like there may be a path more intended by apollo for updating frontend data from the result of a polled query, where updates to data arent explicitly caused by mutations. This might be a good example where a direct cache invalidation may be preferred, but its hard to say. I feel like we should avoid direct calls to the apolloclient so far outside the core components, but i cant come up with any concrete reasons why or why not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seemed to be the most clear route from what I could find but there certainly could be something else out there! I think this pattern is pretty solid for "disjointed" systems where you don't quite know when some sort of data will be available so you just allow the user to call a refresh when they need it 🤷
For DEMOS-1043: this PR removes the debug statements introduced in the previous PR for this effort and reworks the way
refetchQueriesis used in the<AddDocumentDialog>The reason the documents aren't updating is because
refetchQueriesis going to be called as soon as the mutation finishes, but this only puts the document in thedocument_pending_uploadtable and thus re-running the query is going to not have any updateddocumentrecords.We can
useApolloClientto trigger a refetch at an arbitrary time (in this case right after calling the callbackonDocumentUploadSucceeded/ dismissing the modal) - Another one that's kinda tough to test locally so going to test in dev after it's deployed