Skip to content

Bad/out-of-date state in effect teardowns #16019

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

Open
rChaoz opened this issue May 28, 2025 · 12 comments
Open

Bad/out-of-date state in effect teardowns #16019

rChaoz opened this issue May 28, 2025 · 12 comments
Labels
awaiting submitter needs a reproduction, or clarification needs discussion
Milestone

Comments

@rChaoz
Copy link
Contributor

rChaoz commented May 28, 2025

Describe the bug

In short:

let thing = $state(true)

// no longer true!!
thing = false

// later
if (thing) doSomething() // this executes, but it shouldn't

Initially my code had let thing = true. I want to expose the state for use in UI in a template (so I change it to use $state), but this breaks the existing code, as for whatever reason getting the variable returns true even after it's set to false.

Reproduction

https://svelte.dev/playground/f81b82a8cefd45da95d8301e82b0d5d4?version=5.33.4

Update: Better repro - https://svelte.dev/playground/a8c378e462544c9baf1b5f22e0531185?version=5.33.4

Severity

important

Workaround

Currently I use:

let thing = false
let thing2 = $state(false)

function setThing(value) {
  thing = value
  thing2 = value
}

// code keeps using non-reactive state, so it works
if (thing) doSomething()

// in template, where reactivity is needed, use thing2

This is much less than ideal

@rChaoz rChaoz changed the title Swapping = undefined with = $state() breaks code Swapping = true with = $state(true) breaks code May 28, 2025
@dummdidumm
Copy link
Member

@Rich-Harris correct me if I'm wrong but this is expected behavior since #15469 - in the general case it's confusing to get the new value in the teardown.

@rChaoz
Copy link
Contributor Author

rChaoz commented May 28, 2025

That sounds... really bad. Isn't the whole point of Svelte that a reactive variable is just a variable?

Unlike other frameworks you may have encountered, there is no API for interacting with state — count is just a number, rather than an object or a function, and you can update it like you would update any other variable.

[...]

State in Svelte is no different — when you reference something declared with the $state rune, you’re accessing its current value.

So this is just... not true anymore? I'm developing a library function and using the getter pattern described in the docs, which massively advertises states as regular variables:

If add wanted to have access to the current values of a and b, and to return the current total value, you would need to use functions instead

That's exactly what I'm doing, and instead of getting the current value, I'm getting... I don't even know what I'm getting, because I don't know if the library user calls my function in a teardown or not. Am I supposed to just put into the documentation of my function "hey, don't call this function from teardowns"?

As for the point of the PR you mentioned, basically, because it was too difficult for users coming from React to change this code...

element.addEventListener(...)
return () => element.removeEventListener(...)

...into this...

const e = element
e.addEventListener(...)
return e.removeEventListener(...)

...it is now impossible to get the true value of a state (which makes writing library code that works correctly everywhere, including teardowns, impossible). Not to mention, this breaking change was pushed in a non-major version, and without any documentation. I'm terribly sorry for the rant and my emotional reply, but this seems like a massive oversight.

@rChaoz
Copy link
Contributor Author

rChaoz commented May 28, 2025

(sorry again, just want to get the last bit of anger out)

To quote the PR:

$effect(() => {
  const prev = value;
  return () => {
    // could be `true` but is usually `false`, depending on
    // whether the teardown is running because the effect
    // is about to re-run (because `value` changed) or
    // because it's being destroyed (because the
    // component unmounted, for example)
    console.log(prev === value); 
  };
});

Yeah, and this code is also confusing as hell for people coming from React:

const prev = value
doSomething()
// oh not, doSomething might've changed the value, this might be false
console.log(prev === value)

While at it, let's also make it so that the condition above is always true, like it would be in React.

sorry again

@rChaoz
Copy link
Contributor Author

rChaoz commented May 28, 2025

Back to sanity. Here's my (simplified) use-case in short:

class Draggable {
  #dragging = $state(false)

  onDragStart() {
    this.#dragging = true
  }

  onDragEnd() {
    this.#stopDrag()
  }

  #stopDrag() {
    if (this.#dragging) dragFinishCallback()
    this.#dragging = false
  }

  mount(elem) {
    this.elem = elem
    elem.addListeners(...)
  }

  unmount() {
    this.elem.removeListeners(...)

    // don't get stuck in dragging state if element is removed while dragged
    this.#stopDrag()
  }
}

Used something like:

const d = new Draggable()
onMount(() => {
  d.mount(elem)
  return () => d.unmount()
}

And, right now, the dragFinishCallback runs again if the first run causes the element to be unmounted, which is neither intended nor expected.

This is a quite simplified and maybe even a bit of an edge case, but the point stands: with this behaviour, it is impossible to write a standalone reactive class, as you can't guarantee methods are not called from a teardown (one way or another, sometimes through a chain of multiple calls).

@paoloricciuti
Copy link
Member

The point of that or is that in situation like this

{#if obj}
    <Component {obj} />
{/if}

If inside component you have this

$effect(()=>{
    console.log(obj.mystuff);
    return ()=>{
        console.log(obj.mystuff);
    }
});

You generally think that since you guarded the component with an if you are safe...but since that cleanup runs because obj is undefined it actually errors out.

This has some downside in cases like yours where you want to get the very latest value. This is definitely confusing and I wonder if we can find a solution, however: what are you trying to do that you need the value to be the updated one in the teardown?

@rChaoz
Copy link
Contributor Author

rChaoz commented May 28, 2025

@paoloricciuti please see the reply above, posted just seconds before your question

Also, for

$effect(()=>{
    console.log(obj.mystuff);
    return ()=>{
        console.log(obj.mystuff);
    }
})

why not write

$effect(()=>{
    const s = obj.mystuff;
    console.log(s);
    return ()=>{
        console.log(s);
    }
});

and how is it different from

console.log(obj.mystuff);
setTimeout(() => {
  console.log(obj.mystuff);
});

...and why does Svelte need to handle it specifically/differently?

@paoloricciuti
Copy link
Member

@paoloricciuti please see the reply above, posted just seconds before your question

Yeah I was looking at it

@rChaoz
Copy link
Contributor Author

rChaoz commented May 29, 2025

Here's a much better explanation of the issue, and how easily external/library code (a simple sum += value in a .svelte.js file) can break down completely.

The idea is that anytime you have something like:

$effect(() => {
    someLibraryCode()
    return () => someMoreLibraryCode()
})

As long as the code handles reactive state in an async manner (animations, network requests, delays, debouncing...), there is a very high chance it will break, and in a very difficult to track down way. There is nothing the library author can do other than place a Do not call this function in effect teardowns warning in the JSDoc. And even then, the issue can appear if the function is called in another function, and the disclaimer isn't copied over.

@rChaoz rChaoz changed the title Swapping = true with = $state(true) breaks code Bad/out-of-date state in effect teardowns May 29, 2025
@Rich-Harris
Copy link
Member

The prior behaviour caused many bugs and was widely hated. It's not just about appeasing people with terminal React brain, it's the behaviour that pretty much anyone would expect, including TypeScript — the fact that a prop can suddenly appear as undefined in violation of its type is deeply chaotic.

But it never occurred to any of us that someone might be setting state in a teardown function. Setting state in effects is one (strongly discouraged) thing, but setting it in teardown is definitely something I think should be avoided. It might be a good idea if Svelte warned when that happens.

Can you repro the use case in #16019 (comment)? Not quite seeing it from the code, would be interested to understand exactly how it's breaking and what Svelte should be doing differently.

@Rich-Harris Rich-Harris added awaiting submitter needs a reproduction, or clarification needs discussion labels Jun 2, 2025
@Rich-Harris Rich-Harris added this to the 5.x milestone Jun 2, 2025
@rChaoz
Copy link
Contributor Author

rChaoz commented Jun 2, 2025

Here's the repro you asked:

https://svelte.dev/playground/45cf49eb4b714fdabd4a94d4031c21bd?version=5.33.14

Click Show child, then drag the child, and watch the console. The issue occurs on any version after the mentioned PR that added the change, for example on 5.21 (1 version back) it works fine.

My 2 cents on this is that there is nothing obviously wrong with the code:

  • drag & drop functionality is extracted into a separate class
  • when the item is dropped into a correct dropzone, it is moved there, causing the component to be unmounted
  • when the element is unmounted, if it's currently being dragged, set dragged to false (to avoid the case where an element is unmounted due to another reason and it remains stuck forever in dragged state)
  • there is no obvious state set in effect teardown

The idea is that this $effect teardown functionality can introduce very, very subtle bugs due to undocumented behaviour.

For reference, this is the original full drag & drop code - hard to spot the state set in effect teardown here, no?
import { pick } from "$lib/util"
import { onMount, tick } from "svelte"
import { SvelteSet } from "svelte/reactivity"

export type CustomDragEvent = Pick<
    MouseEvent,
    "x" | "y" | "screenX" | "screenY" | "clientX" | "clientY" | "pageX" | "pageY" | "altKey" | "ctrlKey" | "metaKey" | "shiftKey"
>

export interface HasElement {
    element: HTMLElement
}
export type MaybeFunction<T> = T | (() => T)

export interface DraggableCallbacks<DropZone = unknown> {
    /**
     * User has started dragging the element.
     */
    onDragStart(event: CustomDragEvent): void
    /**
     * The element is being dragged.
     * @param event The drag event.
     * @param over The drop-zone(s) the element is currently over.
     */
    onDrag(event: CustomDragEvent, over: DropZone[]): void
    /**
     * The drag has completed, either successfully (dropped) or cancelled.
     * @param event The drag event.
     * @param droppedOver The drop-zone(s) the element was over when it was dropped, if any.
     * @param successfullyDroppedIn The drop-zone the element was dropped in successfully, a `null` value means the drop was unsuccessful.
     * A drop-zone must return `true` from its [onDrop()]{@link DropZoneCallbacks#onDrop} callback for the drop to be considered successful.
     */
    onDragEnd(event: CustomDragEvent, droppedOver: DropZone[], successfullyDroppedIn: DropZone | null): void
}

export interface DropZoneCallbacks<Draggable = unknown> {
    /**
     * A draggable element has entered the drop-zone.
     */
    onDragEnter(event: CustomDragEvent, draggable: Draggable): void
    /**
     * A draggable element is being moved inside the drop-zone, with the pointer position changing.
     */
    onDragOver(event: CustomDragEvent, draggable: Draggable): void
    /**
     * A draggable element has left the drop-zone.
     */
    onDragLeave(event: CustomDragEvent, draggable: Draggable): void
    /**
     * A draggable element was let go on top of the drop-zone.
     * @returns Whether the drop was successful.
     */
    onDrop(event: CustomDragEvent, draggable: Draggable): void | boolean
}

export interface DragDropManagerConfig {
    /**
     * Distance, in pixels, that the mouse must move after starting to hold the primary button, for the click to be registered.
     * @default 10
     */
    mouseMoveThresholdPx?: number
    /**
     * Delay, in milliseconds, after touch start that the touch must be held for dragging to begin.
     * Touch move events during this time will cancel the drag.
     * @default 400
     */
    touchHoldThreshold?: number
    /**
     * Whether to cancel an active drag operation when the `Esc` key is pressed.
     * @default true
     */
    cancelOnEscape?: boolean
    /**
     * Disable `user-select` on the `<body>` element to avoid text selection, during dragging.
     * Applied using the style attribute on the body element.
     * @default true
     */
    disableBodySelect?: boolean
}

/* eslint-disable @typescript-eslint/no-explicit-any */

/**
 * Extract `Draggable` type from a `DragDropManager`
 */
export type ExtractDraggable<T extends DragDropManager<any, any, any, any>> =
    T extends DragDropManager<any, any, infer Draggable, any> ? Draggable : never

/**
 * Extract `DropZone` type from a `DragDropManager`
 */
export type ExtractDropZone<T extends DragDropManager<any, any, any, any>> =
    T extends DragDropManager<any, any, any, infer DropZone> ? DropZone : never

/* eslint-enable */

/**
 * Cross-platform drag & drop solution.
 * Uses both mouse & touch events and simplifies setting up drag & dropping, while providing additional features over traditional HTML5 drag & drop.
 *
 * In the event that a dragged element or hovered drop-zone component is unmounted during a drag operation, then:
 * 1. When unmounting a Draggable, the operation is immediately cancelled.
 *    Events are called as usual (`onDragEnd` for the element, `onDragLeave` for the drop-zones).
 * 2. When unmounting a DropZone, it is treated as if the element left the area, and `onDragLeave` is called for that drop-zone.
 *    No other action is taken.
 *
 * **Note:** For drag events not triggered by pointers (such as the edge-cases described above, and the drag-cancel triggered by the `Esc` key),
 * all coordinates are 0, and all key-hold properties are false.
 */
export class DragDropManager<
    TDraggable = unknown,
    TDropZone = unknown,
    Draggable extends DraggableCallbacks<TDropZone> & HasElement & TDraggable = DraggableCallbacks<TDropZone> & HasElement & TDraggable,
    DropZone extends DropZoneCallbacks<TDraggable> & HasElement & TDropZone = DropZoneCallbacks<TDraggable> & HasElement & TDropZone,
> {
    // Information about current drag. Element being dragged and the zones the pointer is currently over,
    // used to calculate enter, over and leave events.
    // Raw state because we do not care about currentlyOver changing for reactivity, only for drag start/end.
    #draggable = $state.raw<Draggable>()
    readonly #currentlyOver = new SvelteSet<DropZone>()
    #lastDraggable = $state.raw<Draggable>()

    // Info for last events, used for multi-event interactions
    #lastMouseDown: { draggable: Draggable; x: number; y: number } | undefined
    #lastTouchDown: { timeoutId: number; touchId: number } | undefined

    // Attached draggables/drop-zones
    readonly #attachedDraggables = new Map<HTMLElement, Draggable>()
    readonly #attachedDropZones = new Map<HTMLElement, DropZone>()

    constructor(public config: DragDropManagerConfig = {}) {}

    // Getters

    /**
     * Reactive property that indicates whether an element is currently being dragged, same as checking `draggable != null`.
     */
    get dragging() {
        return this.#draggable != null
    }

    /**
     * Reactive property for the currently grabbed element.
     */
    get draggable() {
        return this.#draggable
    }

    /**
     * Same as [`draggable`]{@link #draggable}, except it remains set until the next drag starts (reactive).
     */
    get lastDraggable() {
        return this.#lastDraggable
    }

    /**
     * Allows setting the current `draggable`, with potential to break built-in behaviour.
     *
     * **Should not be used unless you understand the internals of this library and know what you're doing.**
     */
    set draggable(value: Draggable | undefined) {
        this.#draggable = value
    }

    /**
     * Allows setting `lastDraggable` for custom mount animation context.
     *
     * **Should not be used unless you understand the internals of this library and know what you're doing.**
     */
    set lastDraggable(value: Draggable | undefined) {
        this.#lastDraggable = value
    }

    /**
     * Gets the drop-zone(s) the draggable element is currently hovering over, as a [reactive set]{@link SvelteSet}.
     *
     * **The returned set should not be modified.**
     */
    get currentlyOver(): Omit<Set<DropZone>, "add" | "delete" | "clear"> {
        return this.#currentlyOver
    }

    // Draggable callbacks

    #onDraggableDown = (event: MouseEvent | TouchEvent) => {
        const element = event.currentTarget
        if (this.#draggable || !(element instanceof HTMLElement)) return

        // Ignore elements with 'data-no-drag'
        let target = event.target
        while (target != null && target != element.parentElement) {
            if ((target as HTMLElement).dataset.noDrag != undefined) return
            target = (target as HTMLElement).parentElement
        }

        const draggable = this.#attachedDraggables.get(element)!
        if (this.#isTouch(event)) {
            const touch = event.changedTouches.item(0)
            if (touch)
                this.#lastTouchDown = {
                    touchId: touch.identifier,
                    timeoutId: window.setTimeout(() => {
                        if (this.#draggable) return
                        this.#startDrag(draggable, this.#convertEvent(event, touch))
                    }, this.#cfgTouchHoldThreshold),
                }
        } else if (event.button == 0) {
            const element = event.currentTarget as HTMLElement
            this.#lastMouseDown = {
                draggable: this.#attachedDraggables.get(element)!,
                x: event.x,
                y: event.y,
            }
        }
    }

    // TODO is this still needed? why?
    #onDraggableContextMenu = (event: MouseEvent) => {
        if (event.currentTarget instanceof HTMLElement && event.currentTarget.dataset.noDrag != undefined) return
        if (!(event instanceof PointerEvent) || event.pointerType === "touch") event.preventDefault()
    }

    // Document callbacks

    #onDocumentMove = (event: MouseEvent | TouchEvent) => {
        if (!this.#draggable) {
            // Start the drag for mouse events
            if (event instanceof MouseEvent && event.buttons == 1 && this.#lastMouseDown) {
                // distance = sqrt(dx^2 + dy^2), we want distance >= threshold
                // so dx^2 + dy^2 >= threshold^2
                const dx = event.x - this.#lastMouseDown.x
                const dy = event.y - this.#lastMouseDown.y
                if (dx * dx + dy * dy >= this.#cfgMouseMoveThresholdPxSq) {
                    const draggable = this.#lastMouseDown.draggable
                    const dragEvent = this.#convertEvent(event)
                    this.#startDrag(draggable, dragEvent)
                    this.#lastMouseDown = undefined
                    // impossible, just to please TypeScript
                    if (!this.#draggable) return
                } else return
            } else {
                if (this.#lastTouchDown) {
                    window.clearTimeout(this.#lastTouchDown.timeoutId)
                    this.#lastTouchDown = undefined
                }
                return
            }
        }

        if (this.#isTouch(event)) event.preventDefault()
        const [eventData, dragEvent] = this.#prepEvent(event)
        if (eventData == null || dragEvent == null) return
        this.#draggable.onDrag(dragEvent, [...this.#currentlyOver.values()])

        // Check if this event exited any drop-zones it was over before
        const at = document.elementFromPoint(eventData.clientX, eventData.clientY)
        if (!at) return
        const toRemove: DropZone[] = []
        for (const dropZone of this.#currentlyOver) {
            if (dropZone.element.contains(at)) dropZone.onDragOver(dragEvent, this.#draggable)
            else {
                toRemove.push(dropZone)
                dropZone.onDragLeave(dragEvent, this.#draggable)
            }
        }

        // Check if we entered any new drop-zones
        for (const dropZone of this.#attachedDropZones.values()) {
            if (this.#currentlyOver.has(dropZone) || !dropZone.element.contains(at)) continue
            this.#currentlyOver.add(dropZone)
            dropZone.onDragEnter(dragEvent, this.#draggable)
        }

        for (const dropZone of toRemove) this.#currentlyOver.delete(dropZone)
    }

    #onDocumentKeyDown = (event: KeyboardEvent) => {
        if (event.key === "Escape" && this.#draggable && this.#cfgCancelOnEscape) {
            const dragEvent: CustomDragEvent = this.#emptyEvent(pick(event, { altKey: true, ctrlKey: true, metaKey: true, shiftKey: true }))
            this.#stopDrag(dragEvent, null, false)
        }
    }

    #onDocumentUp = (event: MouseEvent | TouchEvent) => {
        this.#lastMouseDown = undefined
        if (!this.#isTouch(event)) this.#setUserSelect(true)
        if (this.#draggable) {
            if (this.#isTouch(event)) event.preventDefault()
            const dragEvent = this.#prepEvent(event)[1]
            if (!dragEvent) return
            let successZone: DropZone | null = null
            for (const dropZone of this.#currentlyOver)
                if (dropZone.onDrop(dragEvent, this.#draggable)) {
                    successZone = dropZone
                    break
                }
            this.#stopDrag(dragEvent, successZone, true)
        } else if (this.#lastTouchDown) {
            window.clearTimeout(this.#lastTouchDown.timeoutId)
            this.#lastTouchDown = undefined
        }
    }

    // Public API

    /**
     * Initialises the DragDropManager by attaching relevant listeners to document.
     */
    init() {
        document.addEventListener("keydown", this.#onDocumentKeyDown)
        document.addEventListener("mouseup", this.#onDocumentUp)
        document.addEventListener("touchend", this.#onDocumentUp)
        document.addEventListener("touchcancel", this.#onDocumentUp)
        document.addEventListener("mousemove", this.#onDocumentMove)
        document.addEventListener("touchmove", this.#onDocumentMove, { passive: false })
    }

    /**
     * Removes all listeners attached by [init()]{@link DragDropManager#init}.
     */
    destroy() {
        document.removeEventListener("keydown", this.#onDocumentKeyDown)
        document.removeEventListener("mouseup", this.#onDocumentUp)
        document.removeEventListener("touchend", this.#onDocumentUp)
        document.removeEventListener("touchcancel", this.#onDocumentUp)
        document.removeEventListener("mousemove", this.#onDocumentMove)
        document.removeEventListener("touchmove", this.#onDocumentMove)
    }

    /**
     * Use to create a DragDropListener in a component. Needs to be called top-level.
     *
     * Attaches the necessary listeners on mount and removes them on unmount.
     */
    static create<
        TDraggable = unknown,
        TDropZone = unknown,
        Draggable extends DraggableCallbacks<TDropZone> & HasElement & TDraggable = DraggableCallbacks<TDropZone> & HasElement & TDraggable,
        DropZone extends DropZoneCallbacks<TDraggable> & HasElement & TDropZone = DropZoneCallbacks<TDraggable> & HasElement & TDropZone,
    >() {
        const instance = new DragDropManager<TDraggable, TDropZone, Draggable, DropZone>()
        onMount(() => {
            instance.init()
            return () => instance.destroy()
        })
        return instance
    }

    /**
     * Attaches to a DOM element that should be draggable.
     *
     * @example
     * // In a Svelte component
     * <script>
     *     let elem;
     *     onMount(() => {
     *         helper.attachDraggable(elem)
     *         return () => helper.detachDraggable(elem)
     *     })
     * </script>
     *
     * <div bind:this={elem}> ... </div>
     */
    attachDraggable(draggable: Draggable) {
        const element = draggable.element
        element.addEventListener("mousedown", this.#onDraggableDown)
        element.addEventListener("touchstart", this.#onDraggableDown)
        element.addEventListener("contextmenu", this.#onDraggableContextMenu)
        this.#attachedDraggables.set(element, draggable)
    }

    /**
     * Detaches from the given DOM element, previously attached with [attachDraggable()]{@link DragDropManager#attachDraggable}.
     */
    detachDraggable(draggable: Draggable) {
        // Ensure the draggable is removed correctly even when its `element` changes
        const element = [...this.#attachedDraggables.entries()].find(([, d]) => d === draggable)?.[0]
        if (!element) {
            console.error("[DragDropManager] Could not detach Draggable - was never attached", draggable)
            return
        }

        // Prevent draggable getting stuck while removed
        if (this.#draggable === draggable) this.#stopDrag(this.#emptyEvent(), null, true)

        element.removeEventListener("mousedown", this.#onDraggableDown)
        element.removeEventListener("touchstart", this.#onDraggableDown)
        element.removeEventListener("contextmenu", this.#onDraggableContextMenu)
        this.#attachedDraggables.delete(element)
    }

    /**
     * Attaches to a DOM element that should be a drop-zone.
     *
     * @example
     * // In a Svelte component
     * <script>
     *     let elem;
     *     onMount(() => {
     *         helper.attachDraggable(elem)
     *         return () => helper.detachDraggable(elem)
     *     })
     * </script>
     *
     * <div bind:this={elem}> ... </div>
     */
    attachDropZone(dropZone: DropZone) {
        this.#attachedDropZones.set(dropZone.element, dropZone)
    }

    /**
     * Detaches from the given DOM element, previously attached with [attachDropZone()]{@link DragDropManager#attachDropZone}.
     */
    detachDropZone(dropZone: DropZone) {
        // Prevent drop zone from getting stuck as hovered
        if (this.#draggable && this.#currentlyOver.has(dropZone)) {
            dropZone.onDragLeave(this.#emptyEvent(), this.#draggable)
            this.#currentlyOver.delete(dropZone)
        }

        this.#attachedDropZones.delete(dropZone.element)
    }

    /**
     * Registers the current component as draggable, calling [attachDraggable()]{@link DragDropManager#attachDraggable} in an {@linkplain $effect}
     * and [detachDraggable()]{@link DragDropManager#detachDraggable} in the teardown.
     * This allows passing a reactive function, or using a reactive getter for the `element` property of the draggable.
     *
     * @example
     *
     * <script>
     *     let elem = $state()
     *     dragDropHelper.registerDraggable(draggable, () => elem)
     * </script>
     *
     * <div bind:this={elem}> ... </div>
     */
    registerDraggable(draggable: Draggable | (() => Draggable | null | undefined)) {
        $effect(() => {
            const d = typeof draggable == "function" ? draggable() : draggable
            const e = d?.element
            if (!d || !e) {
                if (!d) console.warn("[DragDropManager] Could not attach Draggable: draggable is", d)
                else console.warn("[DragDropManager] Could not attach Draggable: element is", e)
                return
            }
            this.attachDraggable(d)
            return () => this.detachDraggable(d)
        })
    }

    /**
     * Registers the current component as a drop-zone, calling [attachDropZone()]{@link DragDropManager#attachDropZone} on mount and
     * [detachDropZone()]{@link DragDropManager#detachDropZone} when the component is unmounted.
     *
     * @example
     *
     * <script>
     *     let elem = $state()
     *     dragDropHelper.registerDropZone(() => {
     *         element: elem,
     *         // callbacks...
     *     })
     * </script>
     *
     * <div bind:this={elem}> ... </div>
     */
    registerDropZone(dropZone: MaybeFunction<DropZone>) {
        $effect(() => {
            const d = typeof dropZone == "function" ? dropZone() : dropZone
            const e = d?.element
            if (!d || !e) {
                if (!d) console.warn("[DragDropManager] Could not attach DropZone: dropZone is", d)
                else console.warn("[DragDropManager] Could not attach DropZone: element is", e)
                return
            }
            this.attachDropZone(d)
            return () => this.detachDropZone(d)
        })
    }

    // Utilities

    #startDrag(draggable: Draggable, event: CustomDragEvent) {
        this.#draggable = draggable
        this.#lastDraggable = draggable
        draggable.onDragStart(event)
        // Find which dropZones the pointer is already over
        this.#currentlyOver.clear()
        const at = document.elementFromPoint(event.clientX, event.clientY)
        if (at != null) {
            for (const [element, dropZone] of this.#attachedDropZones) {
                if (element.contains(at)) {
                    dropZone.onDragEnter(event, draggable)
                    this.#currentlyOver.add(dropZone)
                }
            }
        }
        // Ensure dragging an element won't select random text
        this.#setUserSelect(false)
    }

    #stopDrag(event: CustomDragEvent, successZone: DropZone | null, shouldClearUserSelect: boolean) {
        if (!this.#draggable) return
        for (const callbacks of this.#currentlyOver) callbacks.onDragLeave(event, this.#draggable)
        this.#draggable.onDragEnd(event, [...this.#currentlyOver], successZone)
        this.#draggable = undefined
        this.#currentlyOver.clear()
        if (shouldClearUserSelect) this.#setUserSelect(true)
    }

    #prepEvent(event: TouchEvent | MouseEvent): [MouseEvent | Touch | null, CustomDragEvent | null] {
        if (event instanceof MouseEvent) return [event, this.#convertEvent(event)]
        if (this.#lastTouchDown)
            for (const touch of event.changedTouches) {
                if (touch.identifier == this.#lastTouchDown.touchId) return [touch, this.#convertEvent(event, touch)]
            }
        return [null, null]
    }

    #emptyEvent(event?: Partial<CustomDragEvent>): CustomDragEvent {
        return {
            x: 0,
            y: 0,
            clientX: 0,
            clientY: 0,
            pageX: 0,
            pageY: 0,
            screenX: 0,
            screenY: 0,
            altKey: false,
            ctrlKey: false,
            metaKey: false,
            shiftKey: false,
            ...event,
        }
    }

    #convertEvent(event: MouseEvent): CustomDragEvent
    #convertEvent(event: TouchEvent, touch: Touch): CustomDragEvent
    #convertEvent(event: MouseEvent | TouchEvent, touch?: Touch): CustomDragEvent {
        if (event instanceof MouseEvent) return event as CustomDragEvent
        return {
            x: touch!.clientX,
            y: touch!.clientY,
            pageX: touch!.pageX,
            pageY: touch!.pageY,
            clientX: touch!.clientX,
            clientY: touch!.clientY,
            screenX: touch!.screenX,
            screenY: touch!.screenY,
            ...pick(event, { altKey: true, ctrlKey: true, metaKey: true, shiftKey: true }),
        }
    }

    /**
     * To use instead of `thing instanceof TouchEvent`.
     * The `TouchEvent` global is only defined on touch-enabled devices, so a regular check fails with a {@link ReferenceError}.
     * @see https://bugzilla.mozilla.org/show_bug.cgi?id=1693172
     * @private
     */
    #isTouch(event: TouchEvent | MouseEvent): event is TouchEvent {
        return !!window.TouchEvent && event instanceof TouchEvent
    }

    // Config defaults

    get #cfgTouchHoldThreshold() {
        return this.config.touchHoldThreshold ?? 400
    }

    get #cfgMouseMoveThresholdPxSq() {
        const threshold = this.config.mouseMoveThresholdPx ?? 10
        return threshold * threshold
    }

    get #cfgCancelOnEscape() {
        return this.config.cancelOnEscape ?? true
    }

    #setUserSelect(enabled: boolean) {
        if (this.config.disableBodySelect === false) return
        if (enabled) document.body.style.userSelect = ""
        else document.body.style.setProperty("user-select", "none", "important")
    }
}

In the above (spoiler) code, it is very, very, very (IMO) difficult to realise that changing #draggable: Draggable | undefined to #draggable = $state.raw<Draggable>() will completely break it (which is when I initially created this post, after some furious debugging), as state doesn't act as a regular variable anymore since this teardown change.

@rChaoz
Copy link
Contributor Author

rChaoz commented Jun 3, 2025

Just a thought on the behaviour.

It's not just about appeasing people with terminal React brain, it's the behaviour that pretty much anyone would expect, including TypeScript — the fact that a prop can suddenly appear as undefined in violation of its type is deeply chaotic.

I would argue that this is generally TypeScript's fault (check any of its many open issues on this topic), and the fact that it's way beyond the point of no return for making such breaking changes. For example, TypeScript doesn't find anything wrong with this code, and it's not even async:

let s: string | null = null
s = "hello world" // type is narrowed to `string`

function setNull() { s = null }
setNull()

// type doesn't change, still `string`
console.log(s.length) // no type error, but runtime error is thrown
Same example, in Kotlin

REPL

var s: String? = null
s = "hello world" // type is narrowed to `string`
println(s.length) // this is allowed (no compile error)

fun setNull() { s = null }
setNull()

println(s.length) // but this isn't
// Compile error: Smart cast to 'String' is impossible, because 's' is a local variable that is mutated in a capturing closure.

I don't see why Svelte would try to guarantee something that TypeScript itself can't. At the same time, maybe I find Svelte's behaviour so off because I've mostly worked with multithreaded languages, where even this is not allowed:

if (s != null) s.doSomething()
// Compile error: Smart cast to 'String' is impossible, because 's' is a mutable property that could be mutated concurrently.

I also find Svelte's behaviour very inconsistent (see my earlier reply), where the teardown old value is sometimes not applied, depending on how the state is used in the template. Finally, some people might end up trusting this behaviour so much they will forget that regular variables/objects (or raw state) do not act this way, introducing other bugs.

@gterras
Copy link

gterras commented Jun 3, 2025

I find the sum example more explicit as it only uses code examples from the docs. Maybe it does not match any real-life case but anyone fiddling around in the playground discovering Svelte could stumble on this and be puzzled.

It might be a good idea if Svelte warned when that happens.

Definitely, along some mentions in the docs maybe ? The magic happening behind the scenes could be good to document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting submitter needs a reproduction, or clarification needs discussion
Projects
None yet
Development

No branches or pull requests

5 participants