[GitHub] [superset] michael-s-molina commented on pull request #23488: refactor(sqllab): Remove tableOptions from redux state

["michael-s-molina (via GitHub)" <gi...@apache.org>]

michael-s-molina commented on PR #23488:
URL: https://github.com/apache/superset/pull/23488#issuecomment-1503408108

   @justinpark @eschutho I would like to add my 2 cents about the topic. The motivation for using react-query/RTK Query is the following (copied from [RTK website](https://redux-toolkit.js.org/rtk-query/overview)):
   
   > Over the last couple years, the React community has come to realize that "data fetching and caching" is really a different set of concerns than "state management". While you can use a state management library like Redux to cache data, the use cases are different enough that it's worth using tools that are purpose-built for the data fetching use case.
   
   Originally @justinpark was planning to use [react-query](https://tanstack.com/query/v3/) to manage data fetching and caching. Given that RTK has a tool in the same domain called RTK Query, I advised to [use that instead ](https://github.com/apache/superset/pull/23257#issuecomment-1473898703)to preserve library compatibility.
   
   > Hi @justinpark! What are plans long term for redux here? Are you looking to remove it completely and replace it with react-query or just for select places? I know for a while now we've had state in both redux and in hooks, and haven't ever come to a decision on where frontend state should reside.
   
   RTK Query is built on top of Redux as you can see in another quote from the same RTK website:
   
   > The data fetching and caching logic is built on top of Redux Toolkit's createSlice and createAsyncThunk APIs
   
   What RTK Query does is to automatically generate the actions, reducers, slices, and hooks to manage data fetching and caching. So it's a complement to Redux, not a replacement.
   
   @justinpark volunteered to convert existing `react-query` hooks to RTK Query hooks and remove `react-query` dependency. 
   
   @justinpark I would advise to complete some steps before further modifying existing Redux state:
   - Merge existing PR to [introduce Redux Toolkit](https://github.com/apache/superset/pull/23460)
   - Go back to previous PRs that used `react-query` and replace them with RTK Query
   - Remove `react-query` dependency
   - Before removing any more state from the Redux store, I would model the endpoints based on the API itself (not its current use), to correctly define the parameters and return types, which would give us one of the best benefits of using RTK Query and Typescript: API type safety.
   
   Lastly, thank you so much @justinpark for driving this effort. It's a really nice improvement, long overdue, that will really simplify our use of Redux and introduce many quality checks during development.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org