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

refactor: integrate react-query for execution data fetching #123

Merged
merged 6 commits into from
Nov 19, 2020

Conversation

schottra
Copy link
Contributor

@schottra schottra commented Nov 19, 2020

This is the first in a series of PRs which will update our execution data fetching to use react-query instead of a custom fetch hook and data cache

This change adds react-query and react-query-devtools and updates the hooks used at the top level of the ExecutionDetails page to use react-query for their data mangement.
Specifically, the hook for fetching the top-most Workflow Execution no longer uses the useFetchableData hook, but uses useQuery instead. I also updated the code for terminating an in progress Execution to use a useMutation hook and wait on the execution to reach a terminal state before returning. This was to make sure updates to queried data correctly invalidated queries which were reading that data (and it also happened to expose a subtle issue with the way query keys are generated and hashed).

Extended Details:

  • Added react-query and react-query-devtools. I opted to use the 3.2 beta since it looks fairly stable and will be released soon, and because I had issues creating a query hash override function using the 2.x version.
  • Configured the QueryClient to fail immediately on 401 errors and to serialize non-plain objects using object spread so that we get consistent keys when passing id objects which use prototypes generated by protobufjs.
  • Updated useWorkflowExecution and useTerminateExecutionState to use react queries/mutations.
  • Removed the terminateExecution property from ExecutionContext. This was a hacky solution to allow terminating the execution from deep within the component tree while allowing all consumer of the Execution to see the updated state once termination was complete. This is unnecessary with react-query because all observers of the query will be updated automatically.
  • Added a WaitForQuery component, which is similar to our existing WaitForData component, but is designed to work with the return value from useQuery instead.
  • Updated logic in the ExecutionDetails app bar header to not show the terminate button or extra actions until the execution has actually reached a queued or running state. Terminating an execution while it is in an Unknown state generally does not work correctly.
  • Added a helper function that can be used by promise-based workflows to wait until a Query reaches a desired state. The termination flow uses this to delay returning from the terminateExecution call until the API returns a terminated execution record.
  • Added a useConditionalQuery hook that allows a query to be conditionally executed based on the state of the data. This is used by useWorkflowExecution along with a refreshInterval to instruct react-query to continue fetching the Execution record until it reaches a terminal state (this preserves existing behavior we had around polling executions until they are terminal)
  • Added an observer component that sits at the application level to intercept 401 errors and stop future queries/trigger the login flow. This preserves behavior that was previously handled by useFetchableData.

Copy link
Contributor

@BobNisco BobNisco left a comment

Choose a reason for hiding this comment

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

This is great stuff. Definitely some interesting work going in with the 401 observer, looking forward to seeing how this continues to progress over time 👍

<SystemStatusBanner />
</SkeletonTheme>
</APIContext.Provider>
<ReactQueryDevtools initialIsOpen={false} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this only be enabled in dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the docs, the import is a no-op in production, so this is safe to leave in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try building prod mode locally to verify, and if that's not the case, I'll add logic in the next PR to conditionally render it.

@schottra schottra merged commit 61d9c32 into execution-data-refactor Nov 19, 2020
@schottra schottra deleted the integrate-react-query branch November 19, 2020 21:54
schottra added a commit that referenced this pull request Jan 4, 2021
* refactor: using react-query to load top level Execution

* refactor: upgrading react-query and fixing execution termination

* refactor: handle 401s on queries and do auth flow

* refactor: adding conditional refresh for execution status

* refactor: cleanup broken files after context refactor

* chore: docs
schottra added a commit that referenced this pull request Jan 5, 2021
* refactor: using react-query to load top level Execution

* refactor: upgrading react-query and fixing execution termination

* refactor: handle 401s on queries and do auth flow

* refactor: adding conditional refresh for execution status

* refactor: cleanup broken files after context refactor

* chore: docs
schottra added a commit that referenced this pull request Jan 6, 2021
* refactor: integrate react-query for execution data fetching (#123)

* refactor: using react-query to load top level Execution

* refactor: upgrading react-query and fixing execution termination

* refactor: handle 401s on queries and do auth flow

* refactor: adding conditional refresh for execution status

* refactor: cleanup broken files after context refactor

* chore: docs

* refactor: Remove ExecutionDataCache in favor of react-query (#126)

* refactor: first step of using queries for NE table

* refactor: removing data cache from first layer of NE table

* refactor: removing remaining execution data cache usage

* refactor: rename QueryKey type and remove bug workaround

* refactor: fixing remaining consumers of NEs

* test: adds setup for mock-service-worker (#127)

* test: add msw and basic handlers for a few types

* test: add mock data for a basic workflow execution

* test: fixing/removing tests after adding msw

* test: throw on unexpected requests to msw

* fix: upgrade TS to fix error and cleanup resulting errors

* Migrate from TSLint to ESLint (#128)

* ci: move from tslint->eslint

* fix: addressing eslint errors

* fix: remove passing of unused variable

* ci: remove unnecessary prettier config

* refactor: clean up mock fixtures and re-enable tests for executions (#130)

* test: adding test data for node executions

* test: mocks and refactoring to re-enable NodeExecutionDetails tests

* chore: lint error

* test: getting first test for NE table working again

* test: mocks and a couple of tests for NE table

* refactor: msw handlers to use a backing map and return 404s

* test: more tests for NE table

* test: adding fixture for dynamic external workflow

* test: using mock fixture for sub workflow tests

* test: move remaining mocks to fixtures and fix tests

* test: re-enabling more execution tests

* fix: removing global query handlers for caching entitiesq

* test: re-enable ExecutionNodeViews tests

* fix: typo in import path

* fix: show DataError by default for failed queries

* chore: documentation

* chore: pr feedback

* test: fixing storybook rendering for NE table

* refactor: queries and loading states for (Task)ExecutionDetails

* chore: cleanup unused code

* fix: adds mock support for launch plans

* test: update LoadingSpinner tests

* fix: handle error case for NE children

* chore: remove todo

* fix: typo
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