Skip to content

Conversation

snowystinger
Copy link
Member

Closes

Smallest reproduction: https://codesandbox.io/s/nifty-dewdney-8fbdhi?file=/src/App.js
Notice that after clicking the button we get this in the console

render 0 <div id="target"><button>click</button></div>
user render <div id="target"><button>click</button></div>
assigning to merge ref null
user layouteffect undefined
assigning to merge ref <div id=​"target">​…​</div>​
layouteffect 0 <div id="target"><button>click</button></div>
user effect <div id="target"><button>click</button></div>
effect 0 <div id="target"><button>click</button></div>

that user layouteffect should not be undefined as the component has not been removed during render, however, something about mergeProps causes it to become undefined
I believe this is because mergeProps currently returns a new function each time
I can't really say why that causes the problem, but we can see that using useCallback causes it to behave as expected

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@snowystinger
Copy link
Member Author

I did more research on this, it appears that when a new ref is created (function returned by mergeProps for example), React will set the ref to undefined after render, then it will reassign it just before useLayoutEffect. See https://codesandbox.io/s/wandering-hill-xmhtyz?file=/src/App.js

In addition, refs are assigned in order they are used in the React Tree, not in the order they are defined. In the above example, you'll notice that the ref defined in the App component is always after User but before Field, as the components the ref is applied to appear in the React Tree.

@adobe-bot
Copy link

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Wow... that is some fun TS! 😂

@adobe-bot
Copy link

Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM

@snowystinger snowystinger merged commit d8d0f0b into main Jul 27, 2022
@snowystinger snowystinger deleted the fix-datepicker-overlay-position-nolabel branch July 27, 2022 17:00
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.

4 participants