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

add support for svelte 5 #93

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

itssumitrai
Copy link

add support for svelte 5 by updating peer dependencies

@RonB
Copy link

RonB commented Jul 18, 2024

To really support svelte 5 a refactor of the code should be done I think. I think that svelte 5 components now are classes and not functions. So if for instance rendering special parts of the markdown to SvelteComponents (which I think is brilliant) using tokenizers and renderers, we will need svelte 3 or 4 components, otherwise it will not work.

@itssumitrai
Copy link
Author

To really support svelte 5 a refactor of the code should be done I think. I think that svelte 5 components now are classes and not functions. So if for instance rendering special parts of the markdown to SvelteComponents (which I think is brilliant) using tokenizers and renderers, we will need svelte 3 or 4 components, otherwise it will not work.

Thanks @RonB I will try it out

@memark
Copy link

memark commented Aug 17, 2024

I think you can still use the old class syntax though, right? Just being able to try out Svelte 5 without removing this component would have great value.

@itssumitrai
Copy link
Author

Hi @RonB @memark I got back to finally trying this out on Svelte 5 app, and I can confirm even with Svelte 5, this pkg works.
So, this change is enough to make it work with Svelte 5 projects.

If we wanted a completely svelte 5 project, then there are some changes which need to be done, explicitly old deps need to be updated, and usage of createEventDispatcher need to be removed with a callback prop.

But for now, merging this change to be svelte 5 compatible should be good ?

@itssumitrai
Copy link
Author

cc @pablo-abc I would like to try out svelte 5 for my app, but this dependency is currently blocking. It would be great if we can get this merged. Thanks.

@itssumitrai
Copy link
Author

Sorry for bugging again @pablo-abc but this is a blocker for us to try out svelte 5 for our app, would you be able to take a look ? Thanks

@memark
Copy link

memark commented Oct 23, 2024

Easiest way to Svelte 5 is adding this to your lock package.json file:

  "overrides": {
    "svelte-markdown": {
      "svelte": "^5.0.0"
    }
  }

@RonB
Copy link

RonB commented Oct 23, 2024

Thanks for the tip. Svelte 5.0.0 is released so we can use that for now.
A refractor still is needed i think. How do you feel about that?

@memark
Copy link

memark commented Oct 23, 2024

I'm only upgrading an existing application for a client. I don't know much about this component to be honest.

But as far as I've understood it there are very few breaking changes in Svelte 5. There are some deprecations, but they won't be removed until Svelte 6 (at the earliest).

If you want to upgrade preemptively, here is a guide
https://svelte.dev/docs/svelte/v5-migration-guide

@jycouet
Copy link

jycouet commented Oct 27, 2024

Yep, you be nice to have a release with svelte 5 as valid peer.
Then, in a second effort, embracing runes.

@amosjyng
Copy link

Type checking doesn't work for me on Svelte 5:

Error: Type 'Component<Props, {}, "">' is not assignable to type 'InstantiableSvelteComponentTyped<Partial<Omit<Tokens.Code, "type">>, any, any>'.
  Type 'Component<Props, {}, "">' provides no match for the signature 'new (...args: any[]): SvelteComponentTyped<Partial<Omit<Tokens.Code, "type">>, any, any>'. (ts)
  <div class="markdown">
    <SvelteMarkdown source={message.text} renderers={{ code: CodeRender }} />
  </div>

My code:

<SvelteMarkdown source={message.text} renderers={{ code: CodeRender }} />

where CodeRender is another component I have.

Any ideas?

@memark
Copy link

memark commented Nov 11, 2024

@amosjyng Did this not have any effect for you?
#93 (comment)

@amosjyng
Copy link

@memark I'm using yarn, so I didn't get any conflicts with peer dependencies that required overrides in the first place. My yarn.lock file already only has one version of svelte listed:

svelte@^5.0.0:
  version "5.1.13"
  resolved "https://registry.yarnpkg.com/svelte/-/svelte-5.1.13.tgz#34b5e511d0e2eb36a0f0da77d8d21f3fc143ffe6"
  ...

@jstjoe
Copy link

jstjoe commented Nov 14, 2024

Hey @pablo-abc would you be willing to add someone else as a maintainer? Really appreciate the work you put into this package but it looks like you no longer have time for it - given the last contribution & release in December 2023 and all of the unanswered Issues. Totally understand, life and all. I'm sure there are several of us who would be happy to help keep it going though, as it seems like a lot of us value it and depend on it.

Otherwise unfortunately I think we need to fork this repo to keep it moving forward, folks.

@memark
Copy link

memark commented Nov 14, 2024

Type checking doesn't work for me on Svelte 5:

Error: Type 'Component<Props, {}, "">' is not assignable to type 'InstantiableSvelteComponentTyped<Partial<Omit<Tokens.Code, "type">>, any, any>'.
  Type 'Component<Props, {}, "">' provides no match for the signature 'new (...args: any[]): SvelteComponentTyped<Partial<Omit<Tokens.Code, "type">>, any, any>'. (ts)
  <div class="markdown">
    <SvelteMarkdown source={message.text} renderers={{ code: CodeRender }} />
  </div>

My code:

<SvelteMarkdown source={message.text} renderers={{ code: CodeRender }} />

where CodeRender is another component I have.

Any ideas?

@amosjyng Creating a new project with npx sv create (with yarn) and then using your code line together with the ImageComponent from the docs gives no error. I'm assuming your CodeRenderer is somehow different. What does your CodeRenderer look like? Could you provide an MRE?

@mig-hub
Copy link

mig-hub commented Nov 26, 2024

I can confirm as well that I have a sveltekit app on which I have the latest version of svelte 5 and the latest version of svelte-markdown installed with --legacy-peer-deps, and it is working fine. As mentioned by @jycouet , most things still work with the old syntax (not using runes), so it would be great to just make it compatible first, and then adapt it in a later release.

But this also makes sense if the authors want to adapt it once and for all. Isn't it possible to just run the migration tool?

npx sv migrate svelte-5

I have used it and it does a pretty good job adapting the code.

@mig-hub
Copy link

mig-hub commented Nov 26, 2024

"overrides": {
"svelte-markdown": {
"svelte": "^5.0.0"
}
}

You mean the package.json @memark , not the lock file. Right? I might try this in the meantime. Thank you!

@memark
Copy link

memark commented Nov 26, 2024

@mig-hub Yes, my bad, definately the package.json file. Good catch. I've updated my original post above.

@jacksonthall22
Copy link

Easiest way to Svelte 5 is adding this to your lock package.json file:

  "overrides": {
    "svelte-markdown": {
      "svelte": "^5.0.0"
    }
  }

For pnpm:

  "pnpm": {
    "overrides": {
      "svelte-markdown>svelte": "^5.0.0"
    }
  }

@jycouet
Copy link

jycouet commented Nov 27, 2024

We can always find ways.
I think that the point of @mig-hub is also to know what's the direction of this lib. What's the wish of the author ? Continue ? Give the hand to someone else ? Other ?
We need your input here @pablo-abc

@kimonneuhoff
Copy link

@pablo-abc please answer

@pablo-abc
Copy link
Owner

Hello! I've read through this thread and you're right. This year I've barely had time to open my computer after work and when I have some time I'm focusing mostly on Felte since that's what I'm still using, and I've not been able to use Svelte personally anymore... I'd be fully willing to add someone as a maintainer to this project.

Apologies since on my mind I'd eventually have some time for this, but time flies it seems 🥲

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

Successfully merging this pull request may close these issues.

10 participants