Skip to content

fix: FIx react act warnings #103

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

Merged
merged 2 commits into from
Oct 27, 2024
Merged

fix: FIx react act warnings #103

merged 2 commits into from
Oct 27, 2024

Conversation

taheramr
Copy link
Member

When using test utils in some of our components we get react act warnings, an example of a utility that does that is inside the Autosuggest wrapper wrapper.findAutosuggest().openDrawer().

This happens in all of the components that changes a state like opening a drawer, closing a drawer, or doing an action that will create a transitition effect. The logs of that act warnings:

console.error                                                                                          
    Warning: An update to Transition inside a test was not wrapped in act(...).                          
                                                                                                         
    When testing, code that causes React state updates should be wrapped into act(...):                  
                                                                                                         
    act(() => {                                                                                          
      /* fire events that update state */                                                                
    });                                                                                                  
    /* assert on the output */                                                                           
                                                                                                         

When looking closely, we can notice that the warning is emitted from the setValue state inside the useReducedMotion effect, this is because React does a re-render phase even if the value is the same which is an unexpected behavior.

So for example, if the initial value of the hook is false, and the mutation observer triggered another callback with another value of false, React will reredner the component using that hook, which is the Transition component.

Any following set states with the same value will cause no rerenders in React, this behavior is disccused in great detail in this thread:
https://www.reddit.com/r/reactjs/comments/1ej505e/comment/lths0uh/

To have a resolution, we can manually not update any state if the previous value and the current value is equal inside the hook, this will by defailt fix all of the act warnings stemming from the Transition component.

The solution here might not be ideal, but this is an internal behavior. Following up, I will create an issue with a reproduicble example to the React team and link it here for future work (if needed).

@taheramr taheramr requested a review from a team as a code owner October 24, 2024 14:42
@taheramr taheramr requested review from cansuaa and removed request for a team October 24, 2024 14:42
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.21%. Comparing base (9c4a87e) to head (1ca9e29).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #103   +/-   ##
=======================================
  Coverage   99.21%   99.21%           
=======================================
  Files          27       27           
  Lines         634      636    +2     
  Branches      171      169    -2     
=======================================
+ Hits          629      631    +2     
  Misses          5        5           

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

avinashbot
avinashbot previously approved these changes Oct 25, 2024
* References: https://www.reddit.com/r/reactjs/comments/1ej505e/why_does_it_rerender_even_when_state_is_same/#:~:text=If%20the%20new%20value%20you,shouldn't%20affect%20your%20code
*/
if (newValue !== value) {
setValue(isMotionDisabled(node));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
setValue(isMotionDisabled(node));
setValue(newValue);

you do not need to query the DOM second time

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I missed that for some reason. Updated the code.

@taheramr taheramr added this pull request to the merge queue Oct 27, 2024
Merged via the queue into main with commit 2b188cf Oct 27, 2024
35 checks passed
@taheramr taheramr deleted the fix/fix-react-act-warnings branch October 27, 2024 15:10
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.

5 participants