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: type narrowing for builder props #1015

Merged
merged 11 commits into from
Mar 26, 2024

Conversation

anatolzak
Copy link
Contributor

added as const to every object returned from...

makeElement(name('group'), {
	returned: () => {
		return {
			...
		} as const; // <------
	},
});

Before

Untitled 2

After

Untitled 1

Copy link

changeset-bot bot commented Feb 26, 2024

🦋 Changeset detected

Latest commit: 609671a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@melt-ui/svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@TGlide
Copy link
Member

TGlide commented Feb 26, 2024

I wonder if we can be smarter with this. Using something like this: https://xebia.com/blog/typescript-5-0-and-the-new-const-modifier-on-type-parameters/

@anatolzak
Copy link
Contributor Author

@TGlide that would definitely be preferable! I actually tried adding the const modifier before using as const but haven't had much luck with it so far. Will try to look further into it because that would definitely be the way to go

@anatolzak
Copy link
Contributor Author

anatolzak commented Feb 27, 2024

I tried using the const modifier and was almost able to do it but it seems like TS has a weird bug that breaks its behavior when used in more complex situations.

The objective is for the props returned from builders to be interpreted as const by using the const modifier with generics to avoid adding as const to every single builder.

I created a mini version of the makeElement function and added a new const generic for the props returned from the builder and here are the results. I enabled TypeScript InlayHints to show the types of variables.

Without declaring the store values parameter, the object is interpreted as const. ✅

image

If we declare the store values parameter (most commonly used pattern), the object is no longer interpreted as const. 🚫

image

If we add an explicit type for the store values parameter, the object is interpreted as const. Note that TS already knew the type of the parameter without needing to explicitly define it. But for some reason adding the explicit type, makes the object be interpreted as const. 🚫

image

@anatolzak
Copy link
Contributor Author

@TGlide Unless I am missing something with the const modifier, it seems like it's a little broken and so maybe we should just use as const for now, as it is actually already used in many builders, and then if someone can figure out a better way to do this, they can create a PR

@TGlide TGlide merged commit 8b8531c into melt-ui:develop Mar 26, 2024
4 of 5 checks passed
This was referenced Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants