Unsafe store behaviour of utility components in SvelteKit

[buni]

Current Behavior

Currently components like Toast use exported const stores to function. This is completely fine for all uses cases outside of SvelteKit, as any const variable imported from a library will become global/shared state between requests. This could and will cause unintended behaviour when using SvelteKit.
I haven't spent time to properly test what the impact would be in skeleton's case, so I don't know what would break.

Potential solutions were discussed in discord, also the way Drawers are written should mean that they are safe to use, it looks like only Dialog and Toast are affected.

Steps To Reproduce

Import any const store in sveltekit with SSR enabled.

Anything else?

Dicussion about shared state/stores and SvelteKIt
Issue about shared state behaviour
Another related issue

[endigo9740]

@niktek Tagging you to help review this.

FYI, I was aiming to bring the Drawer in line with Dialog/Toast stores soon, so we need to review the repercussions of this.

[endigo9740]

After reviewing with the team we're going to take the stance that this is important, but not something we can solve easily on our side. If SvelteKit comes out with a hard stance on this or recommend fix or work around then we'll follow suit.

For now I'm going to close this, but I recommend you Buni, or anyone that comes cross this to help keep an eye on progress on this issue.

Thanks for bringing this to our attention!

[DevOfManyThings]

@endigo9740 considering this doesn't look like the svelte team are interested in fixing this issue, would you be interested in me raising a PR using the workaround suggested in this comment: sveltejs/kit#4339 (comment)

Also if this was to be added, probably makes more sense to try and get it in pre 1.0 in case there's any breaking changes (although hopefully it should all just be behind the scenes)

If the svelte team release an official solution for the problem we can obviously update in the future to reflect that, but in the meantime this would at least mean those warnings can be removed from the docs and make Skeleton just a little bit easier to use for devs?

[endigo9740]

@DevOfManyThings I don't think this is a problem Skeleton should aim to solve. We're not Svelte, we're not SvelteKit, we're not going to aim to solve their problems. I'd suggest you try and create a PR to submit TO SvelteKit. But my assumption is they'll get to this, it may just take some time. With SvelteKit v1 out the door I've heard they're turning their attention back to Svelte v4. We should wait and see wha pans out from that.

[DevOfManyThings]

@endigo9740 I know you're sick of the store issue as it's not Skeletons fault, but would you be open to a PR which replaces the official svelte stores for this library? The lib is tiny but solves the issue.

This change would not cause any changes to existing behaviours/functionality, so it wouldn't be a breaking change and means the SSR warnings could be removed, putting this issue to bed permanently.

[endigo9740]

@DevOfManyThings extra dependencies are always going to be the "nuclear" option for us. So this is unlikely.

That said @AdrianGonz97 has mentioned he has some ideas for solving this. I'll let him chime in when he can, but his proposal did sound like it may be a breaking change. So this is not something we would rush in. You're welcome to coordinate with him on this though.

[AdrianGonz97]

@DevOfManyThings Thanks for the suggestion! I took a look at the implementation of package and I don't think it'd be a great fit for us. Unfortunately, the package relies on @sveltejs/kit as a dependency for it to work, which is an immediate no-go for us. Generally, packages should avoid depending on SvelteKit specific modules unless they are meant to only target SvelteKit projects.

Regarding the issue with stores, I'm currently working on an alternative that'll place the responsibility of the store onto the user rather than us handling it for them. I think it should work but it'll be a breaking change and something that would have to be implemented for Skeleton v2.

[DevOfManyThings]

@AdrianGonz97 That dependency is just a type dependency, could quite easily be removed!
In regards to the 3rd party dependency comment from @endigo9740, the logic is quite simple and contained within one file so could be added directly into Skeleton (MIT licenced) if you don't want to introduce a dependency?

I do like the sound of that alternative (I imagine if Skeleton wants to go framework independent then that would also be necessary!) The benefit of this approach is it should be non-breaking so could be added in a minor release as a fix until v2 comes around?

[endigo9740]

This is now resolved and will be a part of the v2 release coming later today.