Svelte: Fix events not being logged in Actions when a story has decorators

[JReinhold]

Closes #20690

What I did

• Calculated Svelte v4/v5 directly in the SlotDecorator component instead of passing it down via props. As [Bug]: Decorators in Svelte causes dispatched events to not be logged in Actions #20690 (comment) correctly points out, SlotDecorator didn't correctly pass that prop down to nested SlotDecorator's. Avoiding prop drilling was just easier
• Moved the conversion of action argTypes to event listeners from the root PreviewRender component down to the inner-most SlotDecorator. Only that last SlotDecorator can actually attach the event listeners to the original story. PreviewRender now passes argTypes down via a context.

Note that there's a weird bug where actions are logged correctly, until you navigate to a story of another component, then all actions will be logged twice, once as click and then as event_click. I have no idea why this happens, I tried to figure it out but couldn't, and it happened before this PR as well, so at least it's not a regression because of this.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

• stories
• unit tests
• integration tests
• end-to-end tests

Manual testing

1. In a Svelte sandbox, open the story at /?path=/story/stories-renderers-svelte-slot-decorators--with-default-red-border (this story has decorators)
2. Click the button
3. See actions logged in the panel.
4. See the currently deployed next sandbox does not log actions, even though it should. https://6308736456ad2046275c0ae7-bczjhlhvbw.chromatic.com/?path=/story/stories-renderers-svelte-slot-decorators--with-default-red-border

Documentation

• Add or update documentation reflecting your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Checklist for Maintainers

• When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

• Make sure this PR contains one of the labels below:

  Available labels

  • bug: Internal changes that fixes incorrect behavior.
  • maintenance: User-facing maintenance tasks.
  • dependencies: Upgrading (sometimes downgrading) dependencies.
  • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
  • cleanup: Minor cleanup style change. Will not show up in release changelog.
  • documentation: Documentation only changes. Will not show up in release changelog.
  • feature request: Introducing a new feature.
  • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
  • other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-28247-sha-834d31db. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information

Published version	0.0.0-pr-28247-sha-834d31db
Triggered by	@JReinhold
Repository	storybookjs/storybook
Branch	jeppe/fix-svelte-decorator-actions
Commit	834d31db
Datetime	Fri Jun 14 22:34:12 UTC 2024 (1718404452)
Workflow run	9523147162

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=28247

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit de8c666. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 1 target

• nx run-many -t build --parallel=3

---

Sent with 💌 from NxCloud.

[kasperpeulen]

What are those 2 variables doing? I don't see them anywhere set in the codebase.

[JReinhold]

They are set in the markup below in the file, and read on line 39.
They are used to listen to events from the instances.

[kasperpeulen]

ah, I see, it is like a react ref?

[JReinhold]

Maybe you're confusing them for props? They aren't exported so they aren't props, they are just regular variables.

[kasperpeulen]

I mean it is like:

const instance = useRef();

<div ref={instance} />

In react?

[JReinhold]

yes you could compare it to a ref, it's just a regular long-living variable.

(my previous message wasn't a response to your react message, we basically posted them at the same time)

[kasperpeulen]

Looking more into the svelte code.

Shouldn't this line be in the SlotDecorator instead?

storybook/code/renderers/svelte/src/components/PreviewRender.svelte

Lines 47 to 56 in 595a08c

	if (!Component) {
	showError({
	title: `Expecting a Svelte component from the story: "${name}" of "${title}".`,
	description: dedent`
	Did you forget to return the Svelte component configuration from the story?
	Use "() => ({ Component: YourComponent, props: {} })"
	when defining the story.
	`,
	});
	}

Normally the component here is the SlotDecorator itself. The error should be there if the user forgets the Component right in a custom render function right?

[kasperpeulen]

// instance can be undefined if a decorator doesn't have a

shouldn't that be an error though?

[JReinhold]

That's debatable, I guess it's similar to not calling storyFn() in a React decorator, to not render the actual story. Would that be invalid? 🤷

[JReinhold]

Shouldn't this line be in the SlotDecorator instead? ... Normally the component here is the SlotDecorator itself. The error should be there if the user forgets the Component right in a custom render function right?

@kasperpeulen

You are right! I tried that, but it's not as easy as I thought with our current data structure. Only the PreviewRender gets the showError-function, and passing that down through all SlotDecorators wasn't as easy as it sounds, because the children could be either SlotDecorators or users' components.

In fact it's also guarding against a very rare case, because we're setting Component in almost all cases here:

storybook/code/renderers/svelte/src/decorators.ts

Lines 44 to 58 in 6dea5b0

	if (!story || Object.keys(story).length === 0) {
	// story is empty or an empty object, use the component from the context
	preparedStory = {
	Component: context.component,
	};
	} else if (story.Component) {
	// the story is already prepared
	preparedStory = story;
	} else {
	// we must assume that the story is a Svelte component
	preparedStory = {
	Component: story,
	};
	}

The only way I could reach the error was by not setting component on meta + not returning anything from render(), which seems very unlikely to me.

I vote leave as-is, the trade off is not there right now.

[kasperpeulen]

This comment seems incorrect.

[kasperpeulen]

LGTM