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: Remove ExecutionDataCache in favor of react-query #126

Merged
merged 5 commits into from
Dec 4, 2020

Conversation

schottra
Copy link
Contributor

@schottra schottra commented Dec 2, 2020

This is another step toward refactoring our data fetching for the ExecutionDetails view to use react-query instead of ExecutionDataCache.
This PR removes ExecutionDataCache and updates all code using it to use queries instead. It also includes a fair amount of supporting code and refactoring to clean up our usage of react-query.

Notes

  • This is being merged into a feature branch. There are currently some TODOs and failed/disabled tests which will be addressed in follow-up PRs.
  • The plan for mocking data in tests/storybook is to switch to using mock-service-worker to mock network requests. The data mocking in tests has always been a little messy/cumbersome. The test philosophy for this repo has leaned more toward integration-style testing, so letting a real network request happen and returning mocked data from a local server is more in line with that philosophy.

Full change list:

  • Updated to react-query v3
  • Refactored code for creating react-query resources
    • Added custom hash function to normalize non-plain objects (for use with protobufJs-generated messages classes in id fields of entities)
    • Added a retry handler which will not retry 401/404 errors.
  • Removed ExecutionDataCache and related context
  • Created nodeExecutionQueries file with queries/hooks related to fetching lists of node executions
  • Updated code for fetching node execution details to use queries
    • Details are fetched for a NodeExecution once and then cached, since they will not change
  • Added queries for Workflows and Tasks
  • Full workflow and task queries will be cached indefinitely because the data is immutable
  • Added global observers for Workflow, NodeExecutionList and TaskExecutionChildList queries to extract nested entities and store them in the query cache. This allows subsequent queries for a single NodeExecution or TaskTemplate to hit the cache instead of requesting data we already have. This was the primary purpose of ExecutionDataCache.
  • Updated ExecutionNodeViews and child components (including DetailsPanel) to be compatible with queries.
  • Updated tests/stories enough to make them compile, disabled suites for any code which previously used ExecutionDataCache. They will need a more significant change to the way data is mocked and will be handled in a future PR.
  • Updated row, child, and cell rendering components for NodeExecutionsTable to get their data from a query, which should be cached from fetching the list at the level above.
  • Removed loading logic from NodeExecutionsTable. There is additional work to make it handle errors and loading states correctly, but it doesn't need full details of how we fetch data.
  • Updated code for fetching NodeExecution children to use queries instead of ExecutionDataCache. This resulted in less-complicated code, so the queries were moved to live inside of nodeExecutionQueries
  • Added QueryType enum for use as the first value in QueryKey arrays. Each type of entity we fetch or generate will have its own type, which is combined with an id and/or RequestConfig object to generate the unique hash for the query.
  • Updated low-level api calls for listing NodeExecutions, TaskExecutions, and their children to have no limit. We usually always want all items with no pagination.
  • General cleanup: removing unused code and imports

@schottra schottra requested review from BobNisco and podehnal December 2, 2020 20:38
@schottra schottra merged commit 54995c8 into execution-data-refactor Dec 4, 2020
@schottra schottra deleted the remove-execution-cache branch December 4, 2020 20:32
schottra added a commit that referenced this pull request Jan 4, 2021
* 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
schottra added a commit that referenced this pull request Jan 5, 2021
* 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
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