-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Move from rest-hooks to react-query #11524
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
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.
(leaving some half-way-through comments, while reviewing the remaining files)
airbyte-webapp-e2e-tests/cypress/support/commands/workspaces.js
Outdated
Show resolved
Hide resolved
useDestinationDefinitionList: () => ({ | ||
destinationDefinitions: [ | ||
{ | ||
destinationDefinitionId: "sid1", |
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.
Trying to understand this change (and the same a few lines below): The tests in this file are all skipped for now anyway, is there any specific reason we changed those IDs to overlap with the sourceDefinitionIds above? Or is this some artifact from trying to get those tests unskipped again?
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 test was disabled before. I tried to make it as close to working as possible, but the issue is that it is difficult to test that useUpdateDestinationDefinition
was called, as we need to mock mutateAsync. I thought, that we need to improve DI a bit, so we could inject DefinitionService
with mocks later and check the call there.
Right now, it is not that easy to fix this test ( or a bit more investigation is required ).
Co-authored-by: Tim Roes <[email protected]>
There is currently one a bit weird behavior, where loading the connection list in one workspace and switching over to another workspace and going back to the connection page (of course works with other resources the same way), will shortly show the connection list of the old workspace before updating them. I think we'll still need to make sure that all queries are invaldiated once the workspace is switched. |
Hmm, indeed, we may need to invalidate all |
I think it would make sense adding the workspace ID to the query key for at least all list queries. That said I'd still additionally invalidate all queries I think when switching, since we have most things scoped to workspaces for now (definitions aren't yet,but that will change soon), so I don't think it's a huge problem invalidating all queries, and leaves us on the safe side of not accidentally missing a query. |
As per our discussion offline - added invalidation of scopes. If required - later workspaceId could be added ( in case smoother behaviour is preferred while switching between workspaces ). Also there was an issue, that queries were reset instead of deleting on changing workspaces. |
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.
Tested every page locally, everything seems to work as expected. Looked through the code, looks good to me.
What
Last piece that finally fixes #10400
rest-hook
library 🎉rest-hook
from tests and storybook/workspaces/get
call instead ofworkspaces/list
call in e2e tests.