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

fix: react hooks observers #55

Merged
merged 13 commits into from
Mar 12, 2024
Merged

fix: react hooks observers #55

merged 13 commits into from
Mar 12, 2024

Conversation

helciofranco
Copy link
Member

@helciofranco helciofranco commented Mar 8, 2024

This PR fixes and reduces a lot of re-renders across our query observers.

This issue was caused due to the way which @tanstack/react-query handles the tracked properties to re-renders.
Internally, they implemented a Proxy to intercept and track which properties a component is listening from a useQuery hook.

It means that when we're calling a useQuery with spread operators, it is forwarding to that Proxy that we want to listen to EVERY existent property.

const query = useQuery();

return { custom: query.data, ...query }; // <- "...query" is the issue here

So, what I did? I created the useNamedQuery that is also a Proxy and it'll also rename the data property to anything you want to name, and keeping the performance everywhere.

This ensures we're going to re-render ONLY when tracked properties get updates.

⚠️ Before ✅ Now
Screenshot 2024-03-08 at 11 13 38 Only 1 property will trigger re-renders Screenshot 2024-03-08 at 11 12 59

Copy link
Contributor

github-actions bot commented Mar 8, 2024

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 87.8% 324/369
🔴 Branches 48.98% 48/98
🟢 Functions 92.86% 91/98
🟢 Lines 89.17% 321/360

Test suite run success

23 tests passing in 3 suites.

Report generated by 🧪jest coverage report action from 072bfb8

@helciofranco helciofranco marked this pull request as ready for review March 8, 2024 15:29
@helciofranco helciofranco self-assigned this Mar 8, 2024
@helciofranco helciofranco linked an issue Mar 8, 2024 that may be closed by this pull request
@luizstacio
Copy link
Member

@helciofranco if I understand the effect of this change, if the users adds a isLoading or other props, them the hook start to track the new properties correct? like { data, isLoading } would track both.

Can we keep the hooks working with named returns? { balance } = useBalance()?

@helciofranco
Copy link
Member Author

@luizstacio that's correct.

From your example, it would re-render only if balance or isLoading got changes.

const { balance, isLoading } = useBalance();

helciofranco added a commit to FuelLabs/fuels-wallet that referenced this pull request Mar 11, 2024
This is exactly the same implementation of
FuelLabs/fuels-npm-packs#55
Applying here just for compatibility purposes (temporarily since this
will be removed soon).

-----

This PR fixes and reduces a lot of re-renders across our query
observers.

This issue was caused due to the way which `@tanstack/react-query`
handles the tracked properties to re-renders.
Internally, they implemented a `Proxy` to intercept and track which
properties a component is listening from a `useQuery` hook.

It means that when we're calling a `useQuery` with spread operators, it
is forwarding to that `Proxy` that we want to listen to EVERY existent
property.

 ```tsx
const query = useQuery();

return { custom: query.data, ...query }; // <- "...query" is the issue
here
```

So, what I did? I created the `useNamedQuery` that is also a `Proxy` and it'll also rename the data property to anything you want to name, and keeping the performance everywhere.

This ensures we're going to re-render **ONLY** when `tracked` properties get updates. 


| ⚠️  Before |  ✅ Now |
| ------ | ------ |
| <img width="288" alt="Screenshot 2024-03-08 at 11 13 38" src="https://github.com/FuelLabs/fuels-npm-packs/assets/7074983/f5aabad7-60ec-4674-bd8a-8b6280c375ef"> | Only 1 property will trigger re-renders <img width="264" alt="Screenshot 2024-03-08 at 11 12 59" src="https://github.com/FuelLabs/fuels-npm-packs/assets/7074983/30ad77ee-399f-44cf-a99f-6af36ae6a0c0"> |
@helciofranco helciofranco merged commit ec20a4b into main Mar 12, 2024
7 checks passed
@helciofranco helciofranco deleted the hf/fix/react-hooks-observers branch March 12, 2024 14:40
helciofranco pushed a commit that referenced this pull request Mar 13, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @fuels/react@0.16.0

### Minor Changes

- 🐞 fix: fuel hooks will only re-render tracked properties, instead of
listening to every useQuery property, by
[@helciofranco](https://github.com/helciofranco) (See
[#55](#55))

## @fuels/assets@0.16.0



## @fuels/changeset@0.16.0



## @fuels/eslint-plugin@0.16.0



## @fuels/jest@0.16.0



## @fuels/local-storage@0.16.0



## @fuels/prettier-config@0.16.0



## @fuels/react-xstore@0.16.0



## @fuels/playwright-utils@0.16.0



## @fuels/ts-config@0.16.0



## @fuels/tsup-config@0.16.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

Improve React Hooks (useQuery) performance
2 participants