-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Bad state in effect teardowns ; non-nullable props are sometimes null #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
Comments
= undefined
with = $state()
breaks code= true
with = $state(true)
breaks code
@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. |
That sounds... really bad. Isn't the whole point of Svelte that a reactive variable is just a variable?
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:
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. |
(sorry again, just want to get the last bit of anger out) To quote the PR:
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 |
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 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). |
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? |
@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? |
Yeah I was looking at it |
Here's a much better explanation of the issue, and how easily external/library code (a simple 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. |
= true
with = $state(true)
breaks code
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 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. |
Here's the repro you asked: https://svelte.dev/playground/45cf49eb4b714fdabd4a94d4031c21bd?version=5.33.14 Click My 2 cents on this is that there is nothing obviously wrong with the code:
The idea is that this 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 |
Just a thought on the behaviour.
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 Kotlinvar 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:
TypeScript doesn't assume it doesn't change until the teardown, so what exactly was the issue? |
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.
Definitely, along some mentions in the docs maybe ? The magic happening behind the scenes could be good to document. |
I'd say rather a tradeoff.. microsoft/TypeScript#9998
This is the issue related to props' types: #14725
Agree.
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. |
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:
I've searched for a bit and it doesn't seem like there are any issues about reading |
Here's another very simple repro: https://svelte.dev/playground/5d82c4c12fae419eabae53d192f37cd8?version=latest This one doesn't even require a 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 guess this example demonstrates a bug in the current behavior...? At least if I understand correctly, what Rich wrote in a comment:
In your repro, changesALot is
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:
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.
Also.. how would that fix the same use case? |
For reference here is how it behaved in Svelte 4. As you can see, it will already have the new value inside 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. |
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 contextAs far as I can tell, the issue original is that, when you have this... <Component prop={value} /> ...the prop actually becomes {#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
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 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 Two cases in which the fix doesn't work: 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 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 NoteThis 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 wordI really didn't like this remark:
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 |
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. My use case is basically this: 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. |
Uh oh!
There was an error while loading. Please reload this page.
Update
It's better to read this comment explaining everything, rather than sift through this entire issue + thread.
Describe the bug
In short:
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 returnstrue
even after it's set tofalse
.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:
This is much less than ideal
The text was updated successfully, but these errors were encountered: