Skip to content

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
@codecov
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