Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

component.$set(props) deprecated in Svelte 5 #14600

Open
TeemuKoivisto opened this issue Dec 7, 2024 · 27 comments
Open

component.$set(props) deprecated in Svelte 5 #14600

TeemuKoivisto opened this issue Dec 7, 2024 · 27 comments

Comments

@TeemuKoivisto
Copy link

TeemuKoivisto commented Dec 7, 2024

Describe the problem

I asked this question on Discord https://discord.com/channels/457912077277855764/1310259142572900363 but since it did not get much attention, I’m asking it here again.

I have a library that mounts Svelte components directly into DOM (related to rich-text) and I manage these components manually. Previously, I could just run new this.component and then this.component.$set(props) whenever the props changed.

With Svelte 5 however, this has been deprecated. There’s no more a $set method on component, frankly the return type of mount often indicates there's nothing on it. I don't know who decided that the return type of mount should be the generic exports property, but it really makes it hard to see directly what's a regular object and what's a mounted Svelte component.

Anyway, so previously $set worked but now the docs suggest that I should use runes instead:

For $set, use $state instead to create a reactive property object and manipulate it. If you’re doing this inside a .js or .ts file, adjust the ending to include .svelte, i.e. .svelte.js or .svelte.ts.

Which apparently sync themselves automatically without ever needing to use $set anymore. Well, this is fine and all but did the authors consider the implications of this matter? Since runes are only allowed within Svelte components or weirdly suffixed x.svelte.ts files, you can't use them in a library that's bundled into single export.

Well, unless you like naming that index.svelte.js as well OR exporting everything separately as ESM. But to me this seems just so hacky and error-prone as there's no guarantees this file-renaming will work flawlessly with the many file-bundling pipelines out there. The fact that you can't, in any shape or form, update the props directly anymore seems weird.

To fix the errors, I ended up switching to svelte/store Writables instead. Which then produces another problem, namely the boilerplate. All my props have now Writable<x> signatures and in general, are much more inconvenient to use than normal TS types.

Describe the proposed solution

What I’d like, is to maintain my previous code (without any writables sprinkled in) but also the file names as they are.

If component.$set is, for some reason, so terrible that it can't be undeprecated, I'd wish there were a way to use runes without the suffixing of files. Just directly importing maybe from somewhere and using them like writables.

Importance

would make my life easier

@paoloricciuti
Copy link
Member

In general you shouldn't bundle your index.svelte.js files: you want the user compiler to compile them or else your code will be stuck at the version you used to compile them.

Regardless of this you can pretty much just build an utility like this

export function with_props(component, props){
    const ret = $state({
        component,
        props,
    });
    return ret;
}

Export it from a .svelte.js file, import it from your .js file and use it there just like this

const counter = with_props(Counter, { count: 0 });
mount(counter.component, { target: div, props: counter.props });

@paoloricciuti
Copy link
Member

example repl

Notice that my_mount is a simple js file

@TeemuKoivisto
Copy link
Author

example repl

In general you shouldn't bundle your index.svelte.js files: you want the user compiler to compile them or else your code will be stuck at the version you used to compile them.

Regardless of this you can pretty much just build an utility like this

export function with_props(component, props){
    const ret = $state({
        component,
        props,
    });
    return ret;
}

Export it from a .svelte.js file, import it from your .js file and use it there just like this

const counter = with_props(Counter, { count: 0 });
mount(counter.component, { target: div, props: counter.props });

First, thanks for the quick reply.

Well, what I mean by bundling is removing the types and joining the source code into a .js single file—not compiling them through Svelte.

I guess that’s better since there’s no Writables but it still means I have to inject a wrapper function to mount my components from the client that uses my Svelte mount-using library. Or then switch to ESM imports and use the .svelte.js suffix for that single wrapper function. Or rename my bundle to index.svelte.js, haven't tried whether that works.

Which I suppose I have to, but it’s a much larger change than what the Svelte v5 let me on. Refactor everything to runes & effect AND change your library exports to ESM. Also, I’m not convinced that the suffix will be errorless given the myriad of file bundlers out there.

@paoloricciuti
Copy link
Member

Or then switch to ESM imports and use the .svelte.js suffix for that single wrapper function

Yeah that's what I meant...browser support for esm modules is everywhere and it's a no brainier using them. Also most likely renaming your bundle should change anything.

Are you still not using ESM?

@TeemuKoivisto
Copy link
Author

Or then switch to ESM imports and use the .svelte.js suffix for that single wrapper function

Yeah that's what I meant...browser support for esm modules is everywhere and it's a no brainier using them. Also most likely renaming your bundle should change anything.

Are you still not using ESM?

I am. But I meant using the "exports" block in the package.json to allow importing them as single ES modules with the .svelte.js suffix. For me, it's often case too much of a headache to export more than a single index.js entrypoint. But I'll try whether changing it to index.svelte.js helps or should I then add more exports.

Also, I want to restate that I think mount should return something else than just {} by default. It's just vague.

@paoloricciuti
Copy link
Member

But I meant using the "exports" block in the package.json to allow importing them as single ES modules with the .svelte.js suffix

That is also something you can do and you don't even have to change the import name, just the reference.

But what I mean is that if you have an .svelte.js that you are importing inside the .js file you don't need to necessarily add that to the exports.

Also I have the feeling this is kinda a XY question. Do you have some code we can look at?

Also, I want to restate that I think mount should return something else than just {} by default. It's just vague.

Components returns what you exports from them. What do you mean vague?

@TeemuKoivisto
Copy link
Author

Well, once I'll try it I can make an example repo later but this is what it looked like with v4:

import type { EditorState } from 'prosemirror-state'
import type { EditorView } from 'prosemirror-view'
import type { SvelteComponent } from 'svelte'

import type { TooltipComponent, TooltipProps } from '../types'

export class SelectionTooltip {
  tooltip: HTMLDivElement
  component?: TooltipComponent
  mounted?: SvelteComponent
  props: TooltipProps

  constructor(view: EditorView, component?: TooltipComponent) {
    this.tooltip = document.createElement('div')
    this.props = {
      from: 0,
      to: 0,
      view,
    }
    this.component = component
    this.tooltip.className = '.ProseMirror-tooltip-wrapper'
    view.dom.parentNode!.appendChild(this.tooltip)
  }

  update(view: EditorView, lastState: EditorState | null) {
    const state = view.state
    if (lastState && lastState.doc.eq(state.doc) && lastState.selection.eq(state.selection)) return
    const { from, to } = state.selection
    this.props = {
      from,
      to,
      view,
    }
    if (this.mounted) {
      this.mounted.$set(this.props)
    } else if (view.editor && this.component) {
      this.mounted = new this.component({
        target: this.tooltip,
        props: this.props
      })
    }
  }

  destroy() {
    this.mounted?.$destroy()
    this.tooltip.remove()
  }
}

To me, quite optimal way of handling Svelte components without any special steps necessary. And this is v5:

import type { EditorState } from 'prosemirror-state'
import type { EditorView } from 'prosemirror-view'
import { mount, unmount } from 'svelte'

import type { TooltipComponent, TooltipProps } from '../types'

export class SelectionTooltip {
  tooltip: HTMLDivElement
  component?: TooltipComponent
  mounted?: {}
  props: TooltipProps

  constructor(view: EditorView, component?: TooltipComponent) {
    this.tooltip = document.createElement('div')
    this.props = {
      from: 0,
      to: 0,
      view,
    }
    this.component = component
    this.tooltip.className = '.ProseMirror-tooltip-wrapper'
    view.dom.parentNode!.appendChild(this.tooltip)
  }

  update(view: EditorView, lastState: EditorState | null) {
    const state = view.state
    if (lastState && lastState.doc.eq(state.doc) && lastState.selection.eq(state.selection)) return
    const { from, to } = state.selection
    this.props = {
      ...this.props,
      from,
      to,
      view,
    }
    if (this.mounted) {
    // @TODO fix
      this.mounted.$set(this.props)
    } else if (view.editor && this.component) {
      this.mounted = mount(this.component, {
        target: this.tooltip,
        props: this.props
      })
    }
  }

  destroy() {
    this.mounted && unmount(this.mounted)
    this.tooltip.remove()
  }
}

@paoloricciuti
Copy link
Member

Yeah in this case I would just make this a .svelte.js file so that you can directly use props as state and simply update the prop that you need

@TeemuKoivisto
Copy link
Author

TeemuKoivisto commented Dec 8, 2024

Yeah in this case I would just make this a .svelte.js file so that you can directly use props as state and simply update the prop that you need

All right here's an example https://github.com/TeemuKoivisto/svelte5-runes-in-libraries-trouble

Good news is that the suffixing works by just renaming the entrypoint to index.svelte.js. Bad news is, and what I feared, that esbuild/vite mangles the class initialization and transforms the original into Svelte5 incompatible version:

import { mount, unmount, type Component } from 'svelte'

interface Props {
  name: string
  time: number
}

export type RenderedComponent = Component<Props>

export class SvelteWrapper {
  dom: HTMLElement
  component?: RenderedComponent
  mounted?: {}
  props: Props = $state({
    name: 'component',
    time: Date.now()
  })

  constructor(dom: HTMLElement, component?: RenderedComponent) {
    this.dom = dom
    this.component = component
  }

  update(time: number) {
    this.props.time = time
    if (this.mounted) {
      // this.mounted.$set(this.props)
    } else if (this.component) {
      this.mounted = mount(this.component, {
        target: this.dom,
        props: this.props
      })
    }
  }

  destroy() {
    this.mounted && unmount(this.mounted)
  }
}

becomes

import { mount, unmount } from "svelte";
class SvelteWrapper {
  constructor(dom, component) {
    this.props = $state({
      name: "component",
      time: Date.now()
    });
    this.dom = dom;
    this.component = component;
  }
  update(time) {
    this.props.time = time;
    if (this.mounted) ;
    else if (this.component) {
      this.mounted = mount(this.component, {
        target: this.dom,
        props: this.props
      });
    }
  }
  destroy() {
    this.mounted && unmount(this.mounted);
  }
}
export {
  SvelteWrapper
};

which crashes with $state(...) can only be used as a variable declaration initializer or a class field

Doing it manually works:

import { mount, unmount } from 'svelte'

export class SvelteWrapper {
  dom
  component
  mounted
  props = $state({
    name: 'component',
    time: Date.now()
  })

  constructor(dom, component) {
    this.dom = dom
    this.component = component
  }

  update(time) {
    this.props.time = time
    if (this.mounted) {
      // this.mounted.$set(this.props)
    } else if (this.component) {
      this.mounted = mount(this.component, {
        target: this.dom,
        props: this.props
      })
    }
  }

  destroy() {
    this.mounted && unmount(this.mounted)
  }
}

And now I have to customize my vite.config? Doesn't seem there's an option to disable this but I could be wrong.

@paoloricciuti
Copy link
Member

Mmm I see you are using vite in lib mode...I don't think it's actually needed. You should let your users bundle your library and not pre bundle it yourself.

@paoloricciuti
Copy link
Member

Try use svelte-package

@TeemuKoivisto
Copy link
Author

I don't think you can export anything without lib mode. With svelte-package (which I just removed) it still produces:

import { mount, unmount } from 'svelte';
export class SvelteWrapper {
    constructor(dom, component) {
        this.props = $state({
            name: 'component',
            time: Date.now()
        });
        this.dom = dom;
        this.component = component;
    }
    update(time) {
        this.props.time = time;
        if (this.mounted) {
            // this.mounted.$set(this.props)
        }
        else if (this.component) {
            this.mounted = mount(this.component, {
                target: this.dom,
                props: this.props
            });
        }
    }
    destroy() {
        this.mounted && unmount(this.mounted);
    }
}

@paoloricciuti
Copy link
Member

You can but your build step should not include vite build. You just need to call svelte package and have the exports in the package.json correctly point to the right file. Svelte package will produce your dist folder, compiling your TS, and creating your .d.ts

@TeemuKoivisto
Copy link
Author

You can but your build step should not include vite build. You just need to call svelte package and have the exports in the package.json correctly point to the right file. Svelte package will produce your dist folder, compiling your TS, and creating your .d.ts

Didn't I just say I ran "build": "svelte-package", without Vite? I mean I know how it works, I have authored a svelte library myself and used it since it first came out. The reason I took it out is because it complicates everything unnecessarily when vite works just fine if I'm not bundling .svelte files.

@paoloricciuti
Copy link
Member

You can but your build step should not include vite build. You just need to call svelte package and have the exports in the package.json correctly point to the right file. Svelte package will produce your dist folder, compiling your TS, and creating your .d.ts

Didn't I just say I ran "build": "svelte-package", without Vite? I mean I know how it works, I have authored a svelte library myself and used it since it first came out. The reason I took it out is because it complicates everything unnecessarily when vite works just fine if I'm not bundling .svelte files.

I don't see how it complicated things: what extra stuff you need to do that you don't need to do with vite? To me it seems vite adds extra stuff to care on because it actually bundles your library

@TeemuKoivisto
Copy link
Author

Dude. Let's not start debating svelte-package. Look, I updated the code here https://github.com/TeemuKoivisto/svelte5-runes-in-libraries-trouble/tree/main/packages/pkg

Isn't that how it's supposed to work? Please, if you can modify the configuration to fix it I'd be delighted.

@paoloricciuti
Copy link
Member

Dude. Let's not start debating svelte-package.

I'm not debating, I'm trying to help you lol.

I'm not at my desk currently, I'll try to see what I can do when I'm back at the desk

@paoloricciuti
Copy link
Member

@TeemuKoivisto what specifically is the problem now?

@TeemuKoivisto
Copy link
Author

@TeemuKoivisto what specifically is the problem now?

Clone that repository. Run pnpm --filter pkg --filter lib build. Then pnpm client. It should show the error

@paoloricciuti
Copy link
Member

Oh ok that's the target in your tsconfig. You can fix it by setting the target to ESNext

@TeemuKoivisto
Copy link
Author

Oh ok that's the target in your tsconfig. You can fix it by setting the target to ESNext

Wow. That's... bizarre. I changed to es2020 since vite/esbuild mangled the class properties by wrapping them with __publicField. This seems quite brittle I must say but I'm happy I got it working. Thanks!

@paoloricciuti
Copy link
Member

Oh ok that's the target in your tsconfig. You can fix it by setting the target to ESNext

Wow. That's... bizarre. I changed to es2020 since vite/esbuild mangled the class properties by wrapping them with __publicField. This seems quite brittle I must say but I'm happy I got it working. Thanks!

it's because TS will change how your class is structured if you use an old enough target.

This is technically a non problem because you just need to publish the library, if the user want to use it with an older target their bundler will do the proper conversion after the svelte compilation but i'm wondering how can we make the experience better

@TeemuKoivisto
Copy link
Author

Oh ok that's the target in your tsconfig. You can fix it by setting the target to ESNext

Wow. That's... bizarre. I changed to es2020 since vite/esbuild mangled the class properties by wrapping them with __publicField. This seems quite brittle I must say but I'm happy I got it working. Thanks!

it's because TS will change how your class is structured if you use an old enough target.

This is technically a non problem because you just need to publish the library, if the user want to use it with an older target their bundler will do the proper conversion after the svelte compilation but i'm wondering how can we make the experience better

Well the problem is that it's too easy to break. The fact this is incompatible with vite is quite befuddling, since any target breaks the original class property setting. I know it's a problem with vite/esbuild but still a rather easy mine to step on.

@paoloricciuti
Copy link
Member

Well the problem is that it's too easy to break. The fact this is incompatible with vite is quite befuddling, since any target breaks the original class property setting. I know it's a problem with vite/esbuild but still a rather easy mine to step on.

It's not incompatible with vite...is just that if you want to build a svelte library you basically need to defer the bundle to their pipeline which is better anyway

@TeemuKoivisto
Copy link
Author

Well the problem is that it's too easy to break. The fact this is incompatible with vite is quite befuddling, since any target breaks the original class property setting. I know it's a problem with vite/esbuild but still a rather easy mine to step on.

It's not incompatible with vite...is just that if you want to build a svelte library you basically need to defer the bundle to their pipeline which is better anyway

I thought we went over this already. So running vite build (with esnext) turns this:

export class Thing {
  name = '123'

  hello() {
    console.log(this.name)
  }
}

into this:

class Thing {
  constructor() {
    __publicField(this, "name", "123");
  }
  hello() {
    console.log(this.name);
  }
}

You do see that the Svelte rune requirement of $state(...) can only be used as a variable declaration initializer or a class field doesn't work with it? Nothing to do with Svelte here, just how vite mangles the class.

@stalkerg
Copy link
Contributor

stalkerg commented Dec 9, 2024

If I understand documentation correctly, currently will be prefer to use Store to a such things. You can put store's as props and modify it any time.

@TeemuKoivisto
Copy link
Author

TeemuKoivisto commented Dec 9, 2024

If I understand documentation correctly, currently will be prefer to use Store to a such things. You can put store's as props and modify it any time.

Well, they are preferable but you need to wrap the props with the Writable/Readable interface which makes them a little clunky. But since you can no longer replace a rune, eg this.props = { name: "thing", time: 1 }, but have to assign each changed property always independently, writables seem a little more ergonomic for this use case.

Also, now I understand why vite/esbuild doesn’t care about the mangling. You can't define values inside class fields that are assigned values inside the constructor. They work for simple use cases where you can just put a default "" or 0, but if you have a complex object, say HTMLElement:

import { mount, unmount, type Component } from 'svelte'

export interface Props {
  name: string
  time: number
}

export type RenderedComponent = Component<Props>

export class SvelteWrapper {
  dom: HTMLElement = $state()
  component?: RenderedComponent
  mounted?: {}
  props: Props = $state({
    name: 'component',
    time: Date.now(),
  })

  constructor(dom: HTMLElement, component?: RenderedComponent) {
    this.dom = dom
    this.component = component
  }

  update(time: number) {
    this.props.time = time
    if (this.mounted) {
      // this.mounted.$set(this.props)
    } else if (this.component) {
      this.mounted = mount(this.component, {
        target: this.dom,
        props: this.props
      })
    }
  }

  destroy() {
    this.mounted && unmount(this.mounted)
  }
}

This doesn’t work because Type 'undefined' is not assignable to type 'HTMLElement' for the $state(). You can just put $state<any>() but that's hacky. So what vite does, is just normalizes the code into the more flexible approach.

BUT, as I tinkered with this, I can set it to a local variable prior to assignment so it becomes:

  constructor(dom: HTMLElement, component?: RenderedComponent) {
    let d = $state(dom)
    this.dom = d
    this.component = component
  }

Which works but is… silly. And now actually the vite build works as well, as apparently this.props = $state({ name: 'component', time: Date.now() }) is akin to setting your house on fire but

    const props = $state({
      name: 'component',
      time: Date.now()
    })
    this.props = props

Is A-ok. I frankly think that the class field requirement in $state(...) can only be used as a variable declaration initializer or a class field is unnecessary as then you have to do weird acrobatics like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants