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

Fix destructured mount in play #271

Open
wants to merge 7 commits into
base: next
Choose a base branch
from

Conversation

SteveALee
Copy link

@SteveALee SteveALee commented Jan 25, 2025

Addresses #270

First time commit to storybook and attempt to fix the svelte play({mount}) problem.

This was a bit tricky given steep learing curve as I went "round the houses" and I'm sure I missed loads of process essentials. If so some mentoring or pointers would help.

The problem was introduced with #169. I could not find a related issue to explain the problem but I guess it's pretty obvious. Hopefully the PR doesn't break anything.

Didn't find any relevent tests but guess some should be added for new code? Here's a story @JReinhold used:

 <Story name="With Mount" play={async ({ mount }) => {
    console.log('pre 1');
    await mount();
    console.log('post 1');
}} />

See comments inline in the commits

@@ -68,7 +68,10 @@ export const createRuntimeStories = (Stories: Component, meta: Meta<Cmp>) => {
* The 'play' function should be delegated to the real play Story function
* in order to be run into the component scope.
*/
storyObj.play = (storyContext) => {
function params(fn) { return fn.toString().match(/[^(]*\(([^)]*)/)?.slice(1) ?? [] }
const isMounting = params(storyObj.play).filter((p) => /\{\s*mount\s*\}/.test(p)).length != 0
Copy link
Author

@SteveALee SteveALee Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think we can import the utils in core used to process {mount} so provided simplified equivalent

storyObj.play = (storyContext) => {
function params(fn) { return fn.toString().match(/[^(]*\(([^)]*)/)?.slice(1) ?? [] }
const isMounting = params(storyObj.play).filter((p) => /\{\s*mount\s*\}/.test(p)).length != 0
storyObj.play = isMounting ? function ({ mount }) { return playDelegator(arguments[0]) } : (storyContext) => playDelegator(storyContext);
Copy link
Author

@SteveALee SteveALee Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used arguments as destructuring ...var rest and then spread to create object many miss any non own non enumerable props.

@SteveALee
Copy link
Author

SteveALee commented Jan 25, 2025

I also noticed,

  • a few typing errors, esp for stories
  • Component is imported in and is svelte 4 only and due to dissapear.
  • TS error hiding is not needed - no such error occured for me

Hope that helps

@SteveALee
Copy link
Author

SteveALee commented Jan 26, 2025

Things to look at when I have more time:

  • Can we pass earler destructured mount detection from core in prepareStory through to this phase and lose extra test on function.toString()?
  • tests

@SteveALee
Copy link
Author

SteveALee commented Jan 28, 2025

Here's a patch for use with patch-package in case anyone else needs this before merge

Save it as patches/@storybook+addon-svelte-csf+5.0.0-next.23

diff --git a/node_modules/@storybook/addon-svelte-csf/dist/runtime/create-runtime-stories.js b/node_modules/@storybook/addon-svelte-csf/dist/runtime/create-runtime-stories.js
index 02dce1f..28381d4 100644
--- a/node_modules/@storybook/addon-svelte-csf/dist/runtime/create-runtime-stories.js
+++ b/node_modules/@storybook/addon-svelte-csf/dist/runtime/create-runtime-stories.js
@@ -55,7 +55,12 @@ export const createRuntimeStories = (Stories, meta) => {
              * The 'play' function should be delegated to the real play Story function
              * in order to be run into the component scope.
              */
-            storyObj.play = (storyContext) => {
+
+          // Steve: manually patched
+            function params(fn) { return fn.toString().match(/[^(]*\(([^)]*)/)?.slice(1) ?? [] }
+            const isMounting = params(storyObj.play).filter((p) => /\{.*,?\s*mount\s*,?.*\}/.test(p)).length != 0
+            storyObj.play = isMounting ? function ({ mount }) { return playDelegator(arguments[0]) } : playDelegator;
+            function playDelegator(storyContext) {
                 const delegate = storyContext.playFunction?.__play;
                 if (delegate) {
                     return delegate(storyContext);

@SteveALee
Copy link
Author

SteveALee commented Jan 29, 2025

I've discovered a problem I can't resolve.

For some reason with the mount test stories the built in mount destructured test is then false and boom, back to the original error described in #270.

I don't understand why this code is returning false.

The problem does not occur the first time you run storybook after npm install so is perhaps something todo with module caching (ie no "Forced re-optimization of dependencies")?

Something may have changed in my config as this suddenly started happen

[UPDATE] It appears to `svelte 5.19.4" as previous 5.19.3 has no such problem. I'm deleting node_modules and reinstalling as I patch the mouting code. This annoying waste of time reminds me why I used to fix dep versions in package.json :)
[Update 2] svelte 5.19.6 is working again

@SteveALee
Copy link
Author

SteveALee commented Jan 29, 2025

Another issues, when I installed the new vitest component tests the "Run Tests" errors on the main test but the expect passes according tothe results panel. The same old mounted error occurs in the console (cmd line not F12)

image

At this point, while I'm happy to work on it with some support, it's probably better to let others pick this PR up. My Hunch is this PR is good but other factors are coming into play and highlighted by this PR. It's hard to follow the program flow.

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.

1 participant