Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
yanick merged 2 commits into testing-library:main from Hagendorn:fix/missing-context-in-types
Oct 13, 2023

Conversation

@Hagendorn
Copy link
Member

@Hagendorn Hagendorn commented Mar 1, 2023

Fixes #212

sebastianrothe, lx4r, basevich, and ion-ciubaciuc reacted with thumbs up emoji
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

Hagendorn reacted with thumbs up emoji
Copy link
Collaborator

I would like to check it with #194

Hagendorn and lx4r reacted with thumbs up emoji

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.

Copy link
Member Author

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

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?

markiewiczart reacted with thumbs up emoji
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
edited
Loading

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!

Copy link

Is there any update on the 'accessors' issue?

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

Reviewers

@sebastianrothe sebastianrothe sebastianrothe left review comments

@yanick yanick yanick requested changes

+2 more reviewers

@ysitbon ysitbon ysitbon left review comments

@jwulf0 jwulf0 jwulf0 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Type error when using context API

AltStyle によって変換されたページ (->オリジナル) /