-
Notifications
You must be signed in to change notification settings - Fork 536
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 HMR errors #5209
base: main
Are you sure you want to change the base?
Fix HMR errors #5209
Conversation
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
🦋 Changeset detectedLatest commit: f3a5ff2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
if (anchorRef.current) { | ||
// Necessary for HMR reloads | ||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
if (anchorRef?.current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, do we know why this ref is undefined when doing our upstream work in dotcom? 🤔 Specifically for HMR. It seems like something is happening with the context value being passed along that's different from how it's being represented in TS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We think it's because the DOM is being replaced, but honestly that's just a theory 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like @camertron said, we think the DOM is being rebuilt and inserted, which temporarily seems to make the refs undefined. However, it's barely undefined long enough to matter. By the time the page renders, everything is rehydrated and all works fine. But for that split second while it's being swapped out, React throws a fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought the ref would be stable since it ultimately is coming from useRef
and the value returned from that wouldn't be undefined
between renders 🤔 (although current
totally would if the DOM is changing)
Could it be that the context value (MenuContext
) is changing during HMR? It seems like we're explicitly casting this value as being defined in:
} = React.useContext(MenuContext) as MandateProps<MenuContextProps, 'anchorRef'> |
Closes #
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist