-
Notifications
You must be signed in to change notification settings - Fork 33
fix: add missing supported Svelte options to SvelteComponentOptions #212 #213
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: add missing supported Svelte options to SvelteComponentOptions #212 #213
Conversation
@sebastianrothe
sebastianrothe
left a comment
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.
Looks good. Better than what was reverted in #198
sebastianrothe
commented
Mar 1, 2023
I would like to check it with #194
cmjoseph07
commented
May 17, 2023
@Hagendorn Is this still in progress? I am still getting the error form #194 and I haven't seen an update on the main branch in over 7 months.
Hagendorn
commented
May 17, 2023
@cmjoseph07 I still wait for feedback. @sebastianrothe did you had the change to check it with #194?
sebastianrothe
commented
May 17, 2023
@cmjoseph07 I still wait for feedback. @sebastianrothe did you had the change to check it with #194?
Ups, I totally forgot. Sorry, everyone. I will give it a try on Thursday.
784a970 to
3379545
Compare
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.
I think 'accessors' should also be in that list of picked options?
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.
Looking at this line and this unit test, I'd say it definetely should be there because it's a valid option; however not as a Pick I think, because it does not exist on ComponentConstructorOptions.
Maybe something like
type SvelteComponentOptions<C extends SvelteComponent> = ComponentProps<C> | Pick<ComponentConstructorOptions<ComponentProps<C>>, "anchor" | "props" | "hydrate" | "intro" | "context"> & { accessors?: boolean }
?
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.
Hey!
It should not. The accessors is not part of the API and does nothing. The test mentioned above works only because it use the <svelte:options accessors /> as L1. See official doc about <svelte:options> here and about svelte/compiler options here.
The accessors config has no impact here. You can set it to false in your test case, it should still pass the test assertions. As mentioned in the doc, Svelte component accessors are built at compile time and not at runtime. For example with a component defined like this:
<script> export let testId; </script> <div data-testid={testId} />
Svelte compiler generates this:
// ...generated code for fragment and instance management // generated class class Component extends SvelteComponent { constructor(options) { super(); init(this, options, instance, create_fragment, safe_not_equal, { testId: 0 }); } } export default Component;
Now, if you add <svelte:options accessors /> to the component, the compiler generates this:
// ...generated code for fragment and instance management // generated class class Component extends SvelteComponent { constructor(options) { super(); init(this, options, instance, create_fragment, safe_not_equal, { testId: 0 }); } get testId() { return this.$$.ctx[0]; } set testId(testId) { this.$$set({ testId }); flush(); } } export default Component;
Hope it helps!
@yanick
yanick
left a comment
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.
We just need to confirm that 'accessors' needs to be added to the line (or not) and I think it's good to be merged!
MircoSteyer
commented
Oct 9, 2023
Is there any update on the 'accessors' issue?
yanick
commented
Oct 13, 2023
Yeah, I think we're good with it. Merging!
Fixes #212