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

Experiment: TanStack Query #39

Draft
wants to merge 13 commits into
base: develop
Choose a base branch
from
Draft

Experiment: TanStack Query #39

wants to merge 13 commits into from

Conversation

szymonkoper
Copy link
Contributor

@szymonkoper szymonkoper commented Aug 14, 2023

Description

It's purpose is a comparison of approaches to networking layer: current one in the project vs using query library.

I left some comments in this PR to help understand those changes.

Changes

Migrated our remote data layer to TanStack Query. Template in this experiment uses useQuery and useMutation. Basically it removed all the boilerplate implemented in template/src/utils/api.ts.

Considerations

TODO:

  • Cover new hooks with unit tests to have the same code functionality and tests coverage as in the codebase before this experiment: useLatestComicQuery, useLogInMutation, useLogOutMutation,
  • Migrate also requests layer to apisauce (it's related to axios) to also remove our boilerplate from template/src/api/common.ts.

Demo

No demo. It looks the same. 🌝

@@ -30,6 +30,7 @@ This app has been generated using [react-native-template-redbeard](https://githu
- `localization/` - Things related to user locale
- `navigation/` - Navigators, routes
- `redux/` - Actions, reducers, sagas
- `remote/` - Remote state (via TanStack Query)
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 indicates remote state (server state) in terms of reflecting inside the app what's stored on some remote server (backend). It's keeping networking state - loading, errors and data from requests and responses.

It's different than redux/, which is client local state, that doesn't need to by synchronised with server. It's updated by local user's action or some effects from remote state updates.

This structure allows to keep them separate.

@@ -29,33 +21,13 @@ interface DemoScreenProps {
}

const DemoScreen = ({ navigation }: DemoScreenProps) => {
const [isLogoutLoading, setIsLogoutLoading] = useState(false)
Copy link
Contributor Author

@szymonkoper szymonkoper Aug 25, 2023

Choose a reason for hiding this comment

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

loading state is handled by TanStack query, see useLatestComicQuery. We don't have to implement our own state here. State of query will be the same (cached) if we call query with the same id from another component.

const dispatch = useAppDispatch()
const { t } = useTranslation()

useEffect(() => {
if (isNotRequested(comicRequest)) {
dispatch(getLatestComicAsync())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to explicitly call initial fetch. It's handled by default by TanStack Query inside useLatestComicQuery

persistor.pause()
await clearPersistence()
dispatch(resetStore())
persistor.persist()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleanup logic moved to useLogOutMutation, which is in template/src/remote/auth.ts.

Also, no need to set loading states, because it's handled internally by lib.

},
getLatestComicAsyncFailure: (state, action: PayloadAction<Error['message']>) => {
state.comic = Failure(action.payload)
},
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 boilerplate of 3 actions for calling request and receiving success/failure was replaced by useQuery.


interface DemoState {
counter: number
comic: RemoteData<Comic, Error['message']>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to store RemoteData values (errors, loading) in our local state, because it's stored internally by useQuery in it's state that represents server state. This way networking doesn't mix with local state (MST or redux).

}

export function* watchGetLatestComicSaga() {
yield* takeEvery(getLatestComicAsync, fetchLatestComic)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sagas logic was replaced by useQuery in useLatestComicQuery

@@ -1,87 +0,0 @@
export enum RemoteDataStates {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole logic from api.ts file, with RemoteData, was replaced by useQuery and useMutation. This logic is in TanStack Query library.

@@ -58,47 +26,5 @@ describe('DemoSlice', () => {

expect(result.counter).toEqual(410)
})

it('set getLatestComicAsync to Loading if there is no comic in store', () => {
Copy link
Contributor Author

@szymonkoper szymonkoper Aug 25, 2023

Choose a reason for hiding this comment

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

This test is removed, because it's testing switching to Loading state when request is started. It's handled by lib now, which is tested separately (in lib).

see isLoading for query: https://tanstack.com/query/v5/docs/react/reference/useQuery
or isPending for mutation: https://tanstack.com/query/v5/docs/react/reference/useMutation

expect(result.comic).toEqual(Loading)
})

it('set getLatestComicAsync to Refreshing if there is already comic in store', () => {
Copy link
Contributor Author

@szymonkoper szymonkoper Aug 25, 2023

Choose a reason for hiding this comment

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

This test is removed, because it's testing switching to Refreshing state when request is started while already having data. It's handled by lib now, which is tested separately (in lib).

See isRefetching -> https://tanstack.com/query/v5/docs/react/reference/useQuery

expect(result.comic).toEqual(Refreshing(comicMockParsed))
})

it('set getLatestComicAsyncSuccess state', () => {
Copy link
Contributor Author

@szymonkoper szymonkoper Aug 25, 2023

Choose a reason for hiding this comment

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

This test is removed, because it's testing switching to Success state when received successful response. It's handled by lib now, which is tested separately (in lib).

See isSuccess in https://tanstack.com/query/v5/docs/react/reference/useQuery

expect(result.comic).toEqual(Success(comicMockResponse))
})

it('set getLatestComicAsyncFailure state', () => {
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 is removed, because it's testing switching to Failure state when received successful response. It's handled by lib now, which is tested separately (in lib).

See isError in https://tanstack.com/query/v5/docs/react/reference/useQuery


describe('DemoSlice', () => {
describe('Demo saga fetchLatestComic', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests of fetchLatestComic are replaced by tests of useLatestComicQuery

} as ReturnType<typeof remoteComics.useLatestComicQuery>)

jest.mock('@remote/auth', () => ({
useLogOutMutation: jest.fn().mockImplementation(() => ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hooks with effects are mocked in component unit tests to avoid unfinished tests (promises running indefinitely).
It's a separate issue that existed here before migration to TanStack Query.

import { mapComic } from '@api/mappers/comicMappers'

export const useLatestComicQuery = () => {
return useQuery({
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 useQuery replaced a lot of boilerplate for fetching the latest comic


export const useLatestComicQuery = () => {
return useQuery({
queryKey: ['comic', 'latest'],
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 key - ['comic', 'latest'] - can be used in another place to get to the same data (cached data, error and loading states)

it('should restore API auth tokens on REHYDRATE auth action', () => {
const rehydrateAction = {
type: REHYDRATE,
key: 'auth',
payload: {
tokens: {
data: fakeAuthTokens,
type: RemoteDataStates.SUCCESS,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need to store and test request state, because it's implemented and tested in useQuery and useMutation

describe('#logInAsyncFailure', () => {
it('should store auth error message', () => {
const state = authSlice.reducer(initialState, logInAsyncFailure(logInErrorMessage))
expect(state.tokens).toEqual(Failure(logInErrorMessage))
Copy link
Contributor Author

@szymonkoper szymonkoper Aug 25, 2023

Choose a reason for hiding this comment

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

Removed those tests because there is no need to test switching states: Loading, Success and Failure, because it's handled by useQuery/useMutation in library

@@ -63,45 +40,13 @@ describe('#watchAuthTokens', () => {
})
})

describe('#watchLogInSaga', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests of logging in logic are moved to tests of useLogInMutation

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.

1 participant