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.

Sign up for GitHub

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

Jump to bottom

fix: "Invariant Violation: Expected targetIds to be registered." #3409

Merged
merged 3 commits into from Mar 29, 2022
Merged

fix: "Invariant Violation: Expected targetIds to be registered." #3409

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99db7b8
fix: "Invariant Violation: Expected targetIds to be registered."
joshkel Mar 22, 2022
f8ba3ee
chore: semver
darthtrevino Mar 28, 2022
c901c10
fix: cancel any raf before creating a new one
darthtrevino Mar 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/backend-html5/src/HTML5BackendImpl.ts
View file
Open in desktop
Expand Up @@ -406,6 +406,9 @@ export class HTML5BackendImpl implements Backend {
this.hoverRafId === null &&
typeof requestAnimationFrame !== 'undefined'
) {
// cancel any existing hover if present
this.cancelHover()
Comment on lines +409 to +410
Copy link

Choose a reason for hiding this comment

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

@darthtrevino this fix that you added is always a no-op since it's inside the if condition—it should be moved before the if.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, thanks - is this even necessary then?


this.hoverRafId = requestAnimationFrame(() => {
if (this.monitor.isDragging()) {
this.actions.hover(dragOverTargetIds || [], {
Copy link

@vsiao vsiao Mar 28, 2022 •
edited

Choose a reason for hiding this comment

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

Thanks for trying to fix this bug! I just commented on https://github.com/react-dnd/react-dnd/pull/3313/files#r836932854 and I think it's still relevant here so I'm cross-linking it. From what I can tell, it still seems very possible to encounter bad race conditions, even with the changes that you've made.

Did you try always calling cancelHover before scheduling to ensure that we're at least capturing the most recent copy of dragOverTargetIds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, @vsiao. So, if I'm understanding correctly:

In the original code, if the user hovers over element A, then quickly hovers over element B before the requestAnimationFrame for A can fire, then the hover will be out of date.

After my changes, I think this won't throw errors, because I think the code is structured to cancel any hover effects before anything that would invalidate a target ID. (Actually, it may be possible for an externally triggered rerender to invalidate a target ID. That may need more investigation.) But it's still a visual bug.

To address that, you're saying we should also cancel the hover effect before triggering any hover effect?

That seems correct to me (and so @darthtrevino's fix should go outside of the if, like you said).

Expand Down