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.

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: "Invariant Violation: Expected targetIds to be registered." #3409

Merged
merged 3 commits into from Mar 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 1 addition & 5 deletions .github/workflows/ci.yml
Expand Up @@ -28,11 +28,7 @@ jobs:
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn-

- run: yarn version check
if: "github.actor != 'dependabot[bot]' && !contains(github.ref , 'release/') && !contains(github.head_ref , 'release/')"
name: Version Check


- run: yarn install
name: Install Dependencies

Expand Down
16 changes: 16 additions & 0 deletions .github/workflows/version-check.yml
@@ -0,0 +1,16 @@
name: Version Check
on: [push, pull_request]
env:
DEFAULT_NODE_VERSION: 16
jobs:
version-check:
runs-on: ubuntu-latest
if: "github.ref != 'main' && github.head_ref != 'main' && !contains(github.ref , 'release/') && !contains(github.head_ref , 'release/') && !contains(github.ref , 'dependabot/') && !contains(github.head_ref , 'dependabot/')"
steps:
- uses: actions/checkout@v3
with:
fetch-depth: 0
- uses: actions/setup-node@v1
with:
node-version: ${{env.DEFAULT_NODE_VERSION}}
- run: yarn version check
9 changes: 9 additions & 0 deletions .yarn/versions/e42d7e63.yml
@@ -0,0 +1,9 @@
releases:
react-dnd-html5-backend: patch

declined:
- react-dnd-documentation
- react-dnd-examples
- test-suite-cra
- test-suite-vite
- react-dnd-test-utils
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
@@ -1,6 +1,6 @@
# Contributing to React-DnD

So you want to contibute to React-DnD? Thank you! This library is a community effort, and your contributions are greatly appreciated.
So you want to contribute to React-DnD? Thank you! This library is a community effort, and your contributions are greatly appreciated.

## FAQ

Expand Down
50 changes: 36 additions & 14 deletions packages/backend-html5/src/HTML5BackendImpl.ts
Expand Up @@ -347,6 +347,7 @@ export class HTML5BackendImpl implements Backend {
if (this.clearCurrentDragSourceNode() && this.monitor.isDragging()) {
this.actions.endDrag()
}
this.cancelHover()
}

private setCurrentDragSourceNode(node: Element | null) {
Expand Down Expand Up @@ -400,6 +401,36 @@ export class HTML5BackendImpl implements Backend {
return false
}

private scheduleHover = (dragOverTargetIds: string[] | null) => {
if (
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

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).

clientOffset: this.lastClientOffset,
})
}

this.hoverRafId = null
})
}
}

private cancelHover = () => {
if (
this.hoverRafId !== null &&
typeof cancelAnimationFrame !== 'undefined'
) {
cancelAnimationFrame(this.hoverRafId)
this.hoverRafId = null
}
}

public handleTopDragStartCapture = (): void => {
this.clearCurrentDragSourceNode()
this.dragStartSourceIds = []
Expand Down Expand Up @@ -429,6 +460,7 @@ export class HTML5BackendImpl implements Backend {
// Avoid crashing if we missed a drop event or our previous drag died
if (this.monitor.isDragging()) {
this.actions.endDrag()
this.cancelHover()
}

// Don't publish the source just yet (see why below)
Expand Down Expand Up @@ -530,6 +562,7 @@ export class HTML5BackendImpl implements Backend {
// Only proceed if we have not handled it already.
this.actions.endDrag()
}
this.cancelHover()
}

public handleTopDragEnterCapture = (e: DragEvent): void => {
Expand Down Expand Up @@ -622,20 +655,7 @@ export class HTML5BackendImpl implements Backend {
this.altKeyPressed = e.altKey
this.lastClientOffset = getEventClientOffset(e)

if (
this.hoverRafId === null &&
typeof requestAnimationFrame !== 'undefined'
) {
this.hoverRafId = requestAnimationFrame(() => {
if (this.monitor.isDragging()) {
this.actions.hover(dragOverTargetIds || [], {
clientOffset: this.lastClientOffset,
})
}

this.hoverRafId = null
})
}
this.scheduleHover(dragOverTargetIds)

const canDrop = (dragOverTargetIds || []).some((targetId) =>
this.monitor.canDropOnTarget(targetId),
Expand Down Expand Up @@ -672,6 +692,7 @@ export class HTML5BackendImpl implements Backend {
if (this.isDraggingNativeItem()) {
setTimeout(() => this.endDragNativeItem(), 0)
}
this.cancelHover()
}

public handleTopDropCapture = (e: DragEvent): void => {
Expand Down Expand Up @@ -709,6 +730,7 @@ export class HTML5BackendImpl implements Backend {
} else if (this.monitor.isDragging()) {
this.actions.endDrag()
}
this.cancelHover()
}

public handleSelectStart = (e: DragEvent): void => {
Expand Down