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

Move from rest-hooks to react-query #11524

Merged
merged 22 commits into from
Apr 1, 2022
Merged

Conversation

jamakase
Copy link
Contributor

@jamakase jamakase commented Mar 29, 2022

What

Last piece that finally fixes #10400

  • removes rest-hook library 🎉
  • removes unnecessary service injections
  • removes rest-hook from tests and storybook
  • sets set initialSetupComplete in /workspaces/get call instead of workspaces/list call in e2e tests.

@github-actions github-actions bot added area/frontend area/platform issues related to the platform labels Mar 29, 2022
@jamakase jamakase self-assigned this Mar 30, 2022
@jamakase jamakase marked this pull request as ready for review March 30, 2022 19:20
@jamakase jamakase requested a review from a team as a code owner March 30, 2022 19:20
@jamakase jamakase changed the title Jamakase/rest hooks refactor Move from rest-hooks to react-query Mar 30, 2022
Copy link
Collaborator

@timroes timroes left a 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)

useDestinationDefinitionList: () => ({
destinationDefinitions: [
{
destinationDefinitionId: "sid1",
Copy link
Collaborator

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?

Copy link
Contributor Author

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 ).

@timroes
Copy link
Collaborator

timroes commented Apr 1, 2022

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.

@jamakase
Copy link
Contributor Author

jamakase commented Apr 1, 2022

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 list queries. Do you think we should prepend all workspace specific queries with workspaceId in filters, or we should just invalidate all queries?

@timroes
Copy link
Collaborator

timroes commented Apr 1, 2022

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.

@jamakase
Copy link
Contributor Author

jamakase commented Apr 1, 2022

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.

Copy link
Collaborator

@timroes timroes left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace rest-hooks by react-query
2 participants