Bad state in effect teardowns ; non-nullable props are sometimes null

[rChaoz]

Update

It's better to read this comment explaining everything, rather than sift through this entire issue + thread.

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

[dummdidumm]

@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]

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]

(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]

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]

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]

@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]

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

Yeah I was looking at it

[rChaoz]

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.

[Rich-Harris]

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.

[rChaoz]

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]

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.

Small update

Came across this error just now:

So, I don't get the statement:

the fact that a prop can suddenly appear as undefined in violation of its type

TypeScript doesn't assume it doesn't change until the teardown, so what exactly was the issue?

[gterras]

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.

[thes01]

I would argue that this is generally TypeScript's fault.

I'd say rather a tradeoff.. microsoft/TypeScript#9998

TypeScript doesn't assume it doesn't change until the teardown, so what exactly was the issue?

This is the issue related to props' types: #14725
Essentialy, when a prop has some narrowed type (e.g. non-nullable), it has that type in the whole component, including the teardown parts.

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

Agree.

Setting state in effects is one (strongly discouraged) thing, but setting it in teardown is definitely something I think should be avoided.

I guess that just happens and sometimes it can be quite non-obvious.

On a side node - in our codebase, we are actually still using a workaround (similar to what @rChaoz has), because I've seen the old values creep in unexpectedly in a situation where they shouldn't. Also I think there's a case where I would expect the prop to be defined, but there's still this pre-#15469 behavior. Haven't had time to investigate/file a PR because it wasn't really easy to find a minimal reproduction.. Which contributes to the point that it's quite hard to track these things down.

[rChaoz]

Seeing why the original change was made, I'm rather surprised this was implemented at the state, not prop level. It seems like the issue was that props explicitly defined as non-nullable are sometimes nullable. #15469 works for effect teardowns, but that's not always when props are accessed (although it's likely the most common place).

It seems like this implementation could (should?) be moved to the prop getter level. And here there are two possibilities:

1. Keep the current implementation, which would retain the issues/bugs mentioned above, but only when accessing props, not any state, taking this from a somewhat rare issue to an extreme edge case, as it requires both changing and then reading the prop in an effect teardown
2. Instead of returning old values in teardowns, how about returning old values if the component is unmounting? I.e. if teardowns are about to be executed because the component will be unmounted, only then the old values should kick in. Additionally, these old values are now kept "forever", so if a timeout executes after a component has unmounted, it too will get the last known prop value.

I've searched for a bit and it doesn't seem like there are any issues about reading undefined for state typed as never-nullish, in non-prop use-cases.

[rChaoz]

Here's another very simple repro: https://svelte.dev/playground/5d82c4c12fae419eabae53d192f37cd8?version=latest

This one doesn't even require a setTimeout. A simple case where this could be encountered is a state for an input field, that should be automatically saved when the component is unmounted. But it isn't. And again, when decoupling logic from components using .svelte.js files, it's very difficult to tell when state might be read in teardowns, or how to avoid it.

Maybe I'm a bit biased, but this issue feels pretty serious to me, with huge potential for subtle bugs, and I'm a bit worried to see no updates on it in over 2 weeks.

[thes01]

I guess this example demonstrates a bug in the current behavior...? At least if I understand correctly, what Rich wrote in a comment:

The guarantee is that the value will be the same in the teardown function as in the effect itself. That's all. So if changesALot updates, the teardown function will see the current value of changesSometimes. If changesSometimes changes — in other words, that change is the reason for the effect to re-run — it will see the prior value.

In your repro, changesALot is dep and changesSometimes is count. So if count wasn't the reason for the effect to update, then we should see the latest value, which we don't.

Maybe I'm a bit biased, but this issue feels pretty serious to me, with huge potential for subtle bugs, and I'm a bit worried to see no updates on it in over 2 weeks.

I know, it seemed to me that way to me, too, in #14725. Especially since this was essentially a breaking change for Svelte 4 compatibility. I guess the solution needs to settle a bit. I believe it won't end up lost forever.

Anyway, regarding your proposals:

Keep the current implementation, which would retain the issues/bugs mentioned above, but only when accessing props, not any state.

Just wondering.. isn't the state actually a prop in your use case "state for an input field, that should be automatically saved when the component is unmounted"? I don't know if I understand correctly how you'd wire up the states/props there.

Instead of returning old values in teardowns, how about returning old values if the component is unmounting?

Also.. how would that fix the same use case?

[dummdidumm]

For reference here is how it behaved in Svelte 4. As you can see, it will already have the new value inside onDestroy when passing the writable via context/prop, but will still have the old value when the subscription happens in the parent already.

So there could be an argument made that the value should be the new one for non-props, or more generally you can expect shallow copies of variables being the previous state but cannot expect the same for properties (since they can be mutated) or variable calls (since the things they use can be changing). I'm not sure though if this can reliably be detected at runtime.

[rChaoz]

I totally understand why the change was made, but I think it introduced undesired behaviour, besides the original fix, which doesn't even work correctly. Also, this is a long post, but please bear with me.

Some context

As far as I can tell, the issue original is that, when you have this...

<Component prop={value} />

...the prop actually becomes () => value, for reactivity (in fact a getter, not a function, but irrelevant here). This seems fine, until you have an {#if} statement:

{#if value != null}
  <Component prop={value} />
{/if}

When the condition becomes false, the component starts unmounting. But, since the prop is really a getter (unlike in Svelte 4), using the prop inside <Component> will return undefined, which is unexpected (and understandably hated behaviour). I think it's really important to note here that, once a component starts unmounting, all effects will cease, so the prop will generally no longer be accessed anyways, except in the following cases:

1. effect teardowns, including return callback of onMount
2. out transition
3. timeouts/async work

The "fix"

This behaviour was "fixed" in #15469. However, out of these 3 instances of the bug occurring, the PR only sets out to fix case number 1. Even more so, it doesn't work correctly; in many cases the fix doesn't work, causing effect teardowns to read the latest state value (the null value for a non-nullable prop). At the same time, the cost/downside of the PR was to completely disallow state reads in any effect teardown, ever, regardless of whether props or {#if} conditions are used, or whether the bug being solved even occurred in the first place.

By disallowing state reads, I mean it is unknown whether you'll be reading the up-to-date value, or an outdated value, in teardowns. And again, state can very easily be read by calling an innocuous-looking function, which then calls another function in a .svelte.js file that handles some global state. So you really have to be extra-careful with effect teardowns, to work around this Svelte bug/limitation.

Two cases in which the fix doesn't work:

• when the value is used in the template - REPL
• when the component has an outro transition - REPL

Even if it did always apply in effect teardowns, it still doesn't guarantee that a prop typed as never-nullable is, in fact, not null, which seemed to be the argument for the change in the first place (Rich - "the fact that a prop can suddenly appear as undefined in violation of its type is deeply chaotic"). This happens, as mentioned before, when working with callbacks, async functions or transitions (REPL).

Do I wish to revert to the original, hated behaviour?

No! Definitely not, I think that non-nullable props should never be null. But the current "fix" (#15469) is simply not the way - not only does it not guarantee that the prop is, in fact, not null, but it also breaks any existing code with effect teardowns that read state - a trade-off that was not considered correctly.

What I wish for, is that the current behaviour is replaced with a proper fix of the original issue, restoring the broken functionality of effect teardowns. My suggestion is to change this...

{#if value != null}
  <Component {value} />
{/if}

...which, right now, compiles into something like this (REPL - check line 18 in the JS output tab)...

const props = {
  get value() { return value }
}
Component($$anchor, { props })

...to compile into this, which would also align with Svelte 4 behaviour, where the prop isn't updated on the component anymore after the {#if} condition becomes false:

const oldProps = { value }
const props = {
  get value() {
    if (componentIsUnmounted) return oldProps.value
    else return (oldProps.value = value)
  }
}
Component($$anchor, { props })

And, of course, remove the oldState behaviour added by the faulty PR.

Update: See a POC of this in #16140, it's nowhere near ideal but should make sense

Note

This is just a demonstration; the code should likely be baked into the component function itself, along the lines of:

// svelte/internal/client
function prepare_props() {
    let unmounted = false
    $.on_unmount(() => unmounted = true)
    const oldProps = { ...props }
    return new Proxy(props, {
        get(target, prop, receiver) {
            if (unmounted && Object.has(oldProps, prop)) return oldProps[prop]
            return Reflect.get(target, prop, receiver);
        },
        set(target, prop, value, receiver) {
            oldProps[prop] = value
            Reflect.set(target, prop, value, receiver);
        }
    })
}

// any component
function Component($$anchor, { props }) {
    props = $.prepare_props(props)
    // ...
}

Final word

I really didn't like this remark:

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.

I understand that it's recommended not to do this, usually to not cause infinite update loops, but it shouldn't be broken. So, I decided to check if state is set in effect teardowns in any official code samples, as I was sure the thing that "would never occur to anyone" is, in fact, officially used. And, quickly enough, I found this in the official Svelte tutorial - modifying a SvelteSet in an effect teardown:

https://github.com/sveltejs/svelte.dev/blob/ec7aae44c8481617daf9e7520567e3565f6169d4/apps/svelte.dev/content/tutorial/02-advanced-svelte/06-context/01-context-api/%2Bassets/app-b/src/lib/Canvas.svelte#L13-L16

[tfedor]

I'll piggyback here as my #16127 was closed as duplicate.

I have encountered this issue yesterday, and I don't feel like I am doing anything that should not be done.
Here's my REPL: https://svelte.dev/playground/83dc6b3a9d964dc9ad9c7514e90839d7?version=5.33.19

My use case is basically this:
I have a selector, where user picks a value. Data are fetched from server, and based on that another part of UI shows.
When user changes the original value, state resets, the UI that was shown is hiddden, we wait for new data to be fetched from server and show that part of the UI again with new data.

I am guarding in the parent component like the colleage above me:

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

has further $derived dependencies based on the value it got from prop.

The entire part of the UI that was toggled had a transition attached, and this broke if the fetching of new data was faster than the transition. Since I was guarding in the parent component, I was never expecting that child can get null values, that broke the $derived.

[rChaoz]

Hi, @tfedor. I started creating a PR to fix this issue (and yours), but it turns out your issue is actually a different bug, so I created #16148 explaining it, if you want to follow it.

Update: Made a PR to fix the issue, you can try your REPL with this PR by changing the version query parameter to pr-16150 in the URL