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

docs(cookbook): network requests recipes #1655

Merged
merged 13 commits into from
Sep 18, 2024

Conversation

vanGalilea
Copy link
Contributor

@vanGalilea vanGalilea commented Aug 28, 2024

Summary

As part of our RNTL Cookbook serie I've composed 'Network Request' recipes for fetch & Axios

Test plan

Sanity check Cookbook app

  • Run the Cookbook app > Click on "Phonebook.."
  • Verify rendering of contact and favorites list

Sanity check recipes docs

  • Run the website app
  • Ensure you can navigate to "Network Requests Recipes"
  • Read and review "Axios" & "Fetch" recipes

Visuals

iOS Android
simulator_screenshot_97B8088A-0622-4AE4-81A6-A6EEE7ABF527 Screenshot_1724824468

Possible spin offs

  • Add an 'msw' recipe under the "Network Requests Recipes" sec.


jest.mock('axios');

jest.setTimeout(10000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When this test suit runs in CI, the default 5s is not sufficient.
Locally it's a matter of 0.91 s.

Any idea what might be causing this on CI? @mdjastrzebski?

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 my measuring:

renders an empty task list ran in 6612ms
PhoneBook fetches contacts successfully and renders in list ran in 7190ms
Top 8 slowest examples (14.071 seconds, 100.0% of total time):
  PhoneBook fetches contacts successfully and renders in list
    7.19 seconds ./app/network-requests/__tests__/PhoneBook.test.tsx
  renders an empty task list
    6.612 seconds ./app/state-management/jotai/__tests__/TaskList.test.tsx
  renders WelcomeScreen in light theme
    0.078 seconds ./app/custom-render/__tests__/index.test.tsx
  PhoneBook fetches favorites successfully and renders all users avatars
    0.0[5](https://github.com/callstack/react-native-testing-library/actions/runs/10590927775/job/29347478417?pr=1655#step:6:6)6 seconds ./app/network-requests/__tests__/PhoneBook.test.tsx
  PhoneBook fails to fetch contacts and renders error message
    0.052 seconds ./app/network-requests/__tests__/PhoneBook.test.tsx
  PhoneBook fails to fetch favorites and renders error message
    0.052 seconds ./app/network-requests/__tests__/PhoneBook.test.tsx
  renders a to do list with 1 items initially, and adds a new item
    0.02[6](https://github.com/callstack/react-native-testing-library/actions/runs/10590927775/job/29347478417?pr=1655#step:6:7) seconds ./app/state-management/jotai/__tests__/TaskList.test.tsx
  renders WelcomeScreen in dark theme
    0.005 seconds ./app/custom-render/__tests__/index.test.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like the spec. tests "renders an empty task list" and "PhoneBook fetches contacts successfully and renders in list" are the slowest as they're the 1st tests in the file 🤷🏻

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.20%. Comparing base (a282d1e) to head (d7acda3).
Report is 71 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1655      +/-   ##
==========================================
- Coverage   95.51%   95.20%   -0.31%     
==========================================
  Files          99       99              
  Lines        5400     5553     +153     
  Branches      857      875      +18     
==========================================
+ Hits         5158     5287     +129     
- Misses        242      266      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Thank you for contributing this PR. Mocking network requests is an essential part of each RN app, so having a clear recipe should really benefit our users.

When mocking fetch or Axios they way you did it, by mocking get/post methods there is an issue where there are 2+ API calls using the same verb and serving proper response based on the URL and/or request body. It seems that the current version of the PR skips this problem by handing two API calls separate, one through fetch and other through axios, while beneficial to showcase these two popopular approaches, it does not show to the user how to solve it.

@pierrezimmermannbam mentioned MSW, which has an elegant solution for this with it's request to response map:
image

Based on this article by Kent C. Dodds, I think mocking fetch is suboptimal and we should explore msw as a primary approach.

examples/cookbook/__mocks__/axios.ts Outdated Show resolved Hide resolved
@vanGalilea vanGalilea marked this pull request as draft September 3, 2024 16:08
} from './test-utils';

describe('PhoneBook', () => {
it('fetches all contacts and favorites successfully and renders lists in sections correctly', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

should the be a mock call here? Seems like the timeout might be caused by different test order execution between the test runs locally and on the CI.

const [favoritesData, setFavoritesData] = useState<User[]>([]);
const [error, setError] = useState<string | null>(null);

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

thinking out loud, perhaps we should showcase usage with TanStack Query instead of manual promise fetching. Implementing data fetching using useEffect to avoid race conditions is verbose. Wdyt @vanGalilea ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Showcase is a very good idea, as it is become and industry standard the last years.
With my approach I intended to keep things more straight-forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me add it up, this week 👍🏻 In the meanwhile, you can review the rest ;)

@vanGalilea vanGalilea marked this pull request as ready for review September 16, 2024 06:26
Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Looks good! Great work @vanGalilea.

Do you want to change anything or can we merge it and publish?

@vanGalilea
Copy link
Contributor Author

Looks good! Great work @vanGalilea.

Do you want to change anything or can we merge it and publish?

Let's merge and I might add Tanstack in the near future to it.

Thanks for the extensive review and thinking along 👍🏻

@mdjastrzebski mdjastrzebski merged commit bd98be0 into callstack:main Sep 18, 2024
9 checks passed
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.

3 participants