Toast store implementation change

[buni]

Before submitting the PR:

• Does your PR reference an issue? If not, please chat to the team on Discord or GitHub before submission.
• Did you update and run tests before submission using npm run test?
• Does your branch follow our naming convention? If not, please amend the branch name using branch -m new-branch-name
• Did you update documentation related to your new feature or changes?

What does your PR address?

Potential "fix" for the problem described in Fixes #522 this shouldn't change the default component behaviour at all. It also allows SvelteKit user to set and manage their own stores, essentially mitigating any shared state concerns. To manage global stores in SvelteKit the best solution is to put them inside component context . This PR has a halfway solution, the best one would be to not have const global stores, but that would change how the components work (you'll always have to bind a store), I assume this is not desired behaviour. One caveat with this solution is that there is still a global variable, but by overriding it all should be fine (I'm not exactly sure how to test variable lifecycles in SvelteKit).

Tips

• Tap "convert to draft" to indicate this is work in progress.
• Link to an issue using the verbiage Fixes #522
• Linked issues will auto-close when the PR is merged.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
skeleton-docs	✅ Ready (Inspect)	Visit Preview	Nov 19, 2022 at 6:41PM (UTC)

[endigo9740]

@buni We're juggling a lot right now, so I may have to leave this PR idle for the time being. I will return and look asap though. Thanks for your patience in the meantime.

[buni]

No worries

[endigo9740]

@buni I've finally had a chance to review this. So what you're proposing is replacing the built-in store with a user-provided store, or at least providing the opportunity for such.

I'm really conflicted on this. Sure it may solve the issue raised, but my concern is how this impacts the DX for Skeleton. We already get a lot of flak for using stores through the library, as misguided as it may be, so I'm not this extra hoop to jump through will sit well with most end users.

I think we're approaching this the wrong way though. Per the original discuss thread on this issue, several Svelte/Kit members summarize this well - it's a usage issue. If users are using stores in this very particular context (SSR/load) then sensitive information can be exposed.

But thinking about it in the context of our library, these stores are used for modals, toasts, and (eventually) drawers. I'm struggling to find a real world use case where folks might submit data in this fashion. I would hope that folks aren't passing sensitive info through these. Additionally, I think rather than trying to solve this problem FOR users, and compromising our DX, we instead take the approach of informing users and let them decide how they want to handle this.

Essentially this may mean that folks can't use these Skeleton features for this particular use case - triggered within a SSR load method. But I kind of feel that's an anti-pattern to begin with. To me, the trigger for these features should only ever happen client-side.

I'm going to open this up to review by a few of the other core contributors. We'll follow the consensus there.

[buni]

In terms of DX, it doesn't really change how the components are used at the moment, you have the option to not pass your own store. It doesn't break the API in that sense. Why I'm concerned about using stores in the out the box way is its wrong, while it might not seriously impact skeleton's components in particular, it can lead to bugs like showing an alert to the wrong user or similar ones.

As a user of the library I would like to be informed about the potential issues with using global variables with SvelteKit, and have an option to not use them. Then I can decide with which approach to go, use the default store or manage my own. Again I really don't see any downside with the way this PR deals with the store issue.

[endigo9740]

I see, so you leave the default store in place and keep it as the default for the prop. Then the prop handles the job from then on. This means the current behavior persist. I follow now - sorry it's been a long week, forgive me heh

In that case this may work. But I think we're both in agreement here that we need more than just the code change, we need this to be written out explicitly in the docs, so users are made aware of the risk. You'll note this is a requirement for any PR we accept per the checklist in the original message above:

Here's the set of requirements I'm going to lay out for this:

• Mirror this change for Modals, since the store works in an identical fashion
• We need to update the Drawer component to include it's own store per Update Drawers to include their own store #528, then follow this pattern as well
• If we're going to use this branch, we need it current with the recent bugfix changes in Fix: Added better types for Toast and fixed issue with actions #587
• Then finally, we need to document the availability and purpose of this change directly in the Usage section of the docs. Probably near the bottom since it's only for select use cases.

I was just about to start work on #528, so I'm going to propose we just go ahead and handle all these changes in a new branch/PR for which I will handle. I'll keep your PR open as reference, but implement all these changes. Once ready I'll close this PR and merge the new changes.

I'll ping you when this new PR is ready for review so you can provide feedback on the changes, doc wording, etc. This means you won't be indicated as a contributor (per GitHub), but I think that's better than asking you to do all this other work. Is that ok with you?

[buni]

Sounds good to me, I'm not too fussed about it, and this PR was always intended as a proposal.

[endigo9740]

Sounds good. Thanks for the proposal then.

Btw, unrelated to above - are you a member on our Discord? If so what's your usename? I chat to so many folks here and there it can sometimes be hard to keep everyone straight. But I was asking specifically as you're a sponsor and I wanted to fill you in on some upcoming organization changes that will affect that. We hav a dedicated channel for sponsors, so I can get you added and explain.

Best we can do until GitHub implement some kind of DM system!

[endigo9740]

@buni I haven't forgot about this stuff. We're just prepping for some big updates dropping Tuesday. As soon as that's out the door this will be one of the first items on my todo list!

[endigo9740]

@buni I'm currently in-progress on the new Drawer rework here:

• Drawer Updates #614

I was just about to implement your suggestion for the store prop to allow users to override this. However, I feel this includes a major issue. We aren't using vanilla stores for Drawers, Modals, and Toasts. We're using modified stores that includes custom methods:

• Toast Store: https://github.com/skeletonlabs/skeleton/blob/dev/src/lib/utilities/Toast/stores.ts
• Modal Store: https://github.com/skeletonlabs/skeleton/blob/dev/src/lib/utilities/Modal/stores.ts
• Drawers Store: (work-in-progress)

It will not be trivial for users to override these, especially the Toast store due to the timeout system. So this is giving me some pause. Technically they could clone and replace our stores. But I'm not sure if everyone will understand how these operate compared to a basic writable store.

I'm going to hold off on this change until we've had more time to review and discuss.

[endigo9740]

@buni FYI I've been discussing this issue with a couple contributors on our Discord. @AdrianGonz97 has pointed out that there is an odd data persistence issue for toasts if you trigger them immediately within a route init. Not even just strictly related to a load function, just a plain old route script tag init.

Refresh this page and you'll briefly see a queue of multiple messages, even though the toasts and store have no persistence built in:

https://global-store-test.vercel.app/
https://github.com/AdrianGonz97/global_toast_store_test/blob/main/src/routes/%2Bpage.svelte#L4

His response explains this well:

so i think it's related to what Buni's issue was talking about. because it's a global store and the toast is being triggered in SSR, the store on the server is mutating which is why we get that persistence across different requests

So for the time being we're leaning towards no changes to our codebase, awaiting an official fix from SK's end, but still adding a message or warning on the docs for Toasts, Dialogs, and Drawers that says something to the effect of:

1. Don't use a global store in a SvelteKit app within a Load function
2. If you need a overlay feature triggered on init, put it inside a Browser context if (browser) ...

Then we can link to the original discussion to provide more context. It's a duct tape fix, but probably the best we can do for the time being. I just really don't think replicating our stores are practical for the majority of our users.

[endigo9740]

I'm going to go ahead and close this PR, but will make reference to it in the upcoming ticket for the proposed messaging change.