-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Svelte: Fix events not being logged in Actions when a story has decorators #28247
Changes from all commits
c621fdc
834d31d
f2901cc
de8c666
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,29 +6,45 @@ | |
export let Component; | ||
export let props = {}; | ||
export let on = undefined; | ||
export let argTypes = undefined; | ||
|
||
let instance; | ||
let decoratorInstance; | ||
Comment on lines
11
to
12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are those 2 variables doing? I don't see them anywhere set in the codebase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are set in the markup below in the file, and read on line 39. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, I see, it is like a react ref? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe you're confusing them for props? They aren't exported so they aren't props, they are just regular variables. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean it is like: const instance = useRef();
<div ref={instance} /> In react? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
|
||
const svelteVersion = VERSION[0]; | ||
|
||
/* | ||
Svelte Docgen will create argTypes for events with the name 'event_eventName' | ||
The Actions addon will convert these to args because they are type: 'action' | ||
We need to filter these args out so they are not passed to the component | ||
*/ | ||
let propsWithoutDocgenEvents; | ||
$: propsWithoutDocgenEvents = Object.fromEntries( | ||
Object.entries(props).filter(([key]) => !key.startsWith('event_')) | ||
); | ||
|
||
if (on && svelteVersion < 5) { | ||
if (argTypes && svelteVersion < 5) { | ||
const eventsFromArgTypes = Object.fromEntries( | ||
Object.entries(argTypes) | ||
.filter(([key, value]) => value.action && props[key] != null) | ||
.map(([key, value]) => [value.action, props[key]]) | ||
); | ||
// Attach Svelte event listeners in Svelte v4 | ||
// In Svelte v5 this is not possible anymore as instances are no longer classes with $on() properties, so it will be a no-op | ||
onMount(() => { | ||
Object.entries(on).forEach(([eventName, eventCallback]) => { | ||
// instance can be undefined if a decorator doesn't have <slot/> | ||
Object.entries({ ...eventsFromArgTypes, ...on }).forEach(([eventName, eventCallback]) => { | ||
// instance can be undefined if a decorator doesn't have a <slot/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
shouldn't that be an error though? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's debatable, I guess it's similar to not calling |
||
const inst = instance ?? decoratorInstance; | ||
inst?.$on?.(eventName, eventCallback) | ||
inst?.$on?.(eventName, eventCallback); | ||
}); | ||
}); | ||
} | ||
</script> | ||
|
||
{#if decorator} | ||
<svelte:component this={decorator.Component} {...decorator.props} bind:this={decoratorInstance}> | ||
<svelte:component this={Component} {...props} bind:this={instance} /> | ||
<svelte:component this={Component} {...propsWithoutDocgenEvents} bind:this={instance} /> | ||
</svelte:component> | ||
{:else} | ||
<svelte:component this={Component} {...props} bind:this={instance} /> | ||
<svelte:component this={Component} {...propsWithoutDocgenEvents} bind:this={instance} /> | ||
{/if} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems incorrect.