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: add missing supported Svelte options to SvelteComponentOptions #212 #213

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

Hagendorn
Copy link
Member

Fixes #212

Copy link
Collaborator

@sebastianrothe sebastianrothe left a 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
Copy link
Collaborator

I would like to check it with #194

@cmjoseph07
Copy link

@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
Copy link
Member Author

@cmjoseph07 I still wait for feedback. @sebastianrothe did you had the change to check it with #194?

@sebastianrothe
Copy link
Collaborator

@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.

@yanick yanick force-pushed the fix/missing-context-in-types branch from 784a970 to 3379545 Compare June 7, 2023 14:00

export * from '@testing-library/dom'

type SvelteComponentOptions<C extends SvelteComponent> = ComponentProps<C> | {props: ComponentProps<C>}
type SvelteComponentOptions<C extends SvelteComponent> = ComponentProps<C> | Pick<ComponentConstructorOptions<ComponentProps<C>>, "anchor" | "props" | "hydrate" | "intro" | "context">
Copy link
Collaborator

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?

Copy link

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 }

?

Copy link
Contributor

@ysitbon ysitbon Jul 16, 2023

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!

Copy link
Collaborator

@yanick yanick left a 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
Copy link

Is there any update on the 'accessors' issue?

@yanick
Copy link
Collaborator

yanick commented Oct 13, 2023

Yeah, I think we're good with it. Merging!

@yanick yanick merged commit 800e33e into testing-library:main Oct 13, 2023
5 checks passed
@github-actions
Copy link

🎉 This PR is included in version 4.0.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type error when using context API
7 participants