storybookjs / addon-svelte-csf

[Incubation] CSF using Svelte components.
MIT License
98 stars 29 forks source link

fix: keep track of arguments from a separate map to have the last instance of the component #163

Closed paoloricciuti closed 5 months ago

paoloricciuti commented 5 months ago

THE PROBLEM

At the end of the day storybook expects a CSF which is structured something like this

export default {
// meta
}

export const StoryOne = {
// storyone args
}

export const StoryTwo = {
// storytwo args
}

to achieve this the vite plugin transform the .stories.svelte component and make use of the parse function from collect-stories.ts that instantiate a "headless" version of the component to collect this information (very clever trick btw).

The problem with this approach however is that if there are closures in those parameters the scope over which they close is the one from the headless story and not the one actually rendered. This means that if you try to modify state from within the play function it will not be reflected in the actual mounted story. This is especially annoying if you have to use timing functions (setInterval or setTimeout) because they need to be cleaned up but the onDestroy from svelte doesn't have access to the same scope of the play function

Eg.

<script>
  let timeout;

  onDestroy(()=>{
    clearTimeout(timeout); // timeout is always undefined
  });
</script>

<Story play={()=>{
  timeout = setTimeout(()=>{}, 1000);
}}>
<Button />
</Story>

THE SOLUTION

To fix this issue i made two small changes: i created a map of stories_name => stories arguments inside the context and when a Story is rendered it update this map. This Map is then imported from collect-stories.ts and, if the argument is a function, the actual argument will get the actual argument from the map and call it (passing over every prop). This will ensure that the function will always be the one of the latest render of the story.

I don't know if this is the right approach and i would love some feedback on it from someone with more experience on this addon.

paoloricciuti commented 5 months ago

tbh I don't have confidence in these changes. It's too hacky for me ^^

I would prefer I think a special case for the "play" function using maybe the storyContext to get the real function. I didn't test this path however.

Sure if you can point me towards how to use the storyContext to get the real function I would gladly implement this change. Also, the shared map should not be a problem because even if something gets override it will be overridden again during the render of the current story.

Open to discussion for the best solution tho.