sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.22k stars 4.09k forks source link

Add a (singular) `$prop` rune #9241

Closed aradalvand closed 3 weeks ago

aradalvand commented 11 months ago

Describe the problem

This came up multiple times in the Discord server, so I figured I'd create an issue for it: In v5's runes mode, the only way to declare props is by using the $props rune, like so:

let { someProp = 'default' } = $props();

This looks good unless you're using TypeScript, in which case in order to type your props, you'll have to pass a type argument to $props, what this means in effect is that you'll be duplicating your prop names:

let { someProp = 'default' } = $props<{ someProp: string }>();

Needless to say this is not very pretty, feels a bit overly verbose, it gets even more problematic when you have many props:

let {
    foo = 'default',
    bar,
    buzz = 123,
} = $props<{
    foo: string;
    bar: number;
    buzz: number;
}>();

Here, you have to traverse quite a bit of distance with your eyes in order to find a specific prop's type (e.g. bar), because the destructured variable and the corresponding type could potentially be far from each other, and you'll have to "switch" your eyes to an entirely different "list", if you will.

In addition, this format yields a weird order of name => default-value => name => type, and when you compare all of this to the old way of declaring props with types, the difference becomes apparent:

export let foo: string = 'default';
export let bar: number;
export let buzz: number = 123;

Which is more concise, of course, but the order is more logical as well: name => type => default-value.

Describe the proposed solution

Introduce a singular $prop rune, like so:

let someProp = $prop<string>('default');

Which, once again, is less verbose and has a more logical order of symbols; the type of each prop is always next to it on the same line, and you don't have to repeat the prop name.

Alternatives considered

Importance

nice to have

theodorejb commented 11 months ago

Would it be possible for this to work with a type declaration directly on the variable rather than a generic? E.g.:

let someProp: string = $prop('default');
Snailedlt commented 11 months ago

If "hijacking" TypeScript's syntax is an option, I also think this should be a considered syntax:

let {
    foo: string = 'default',
    bar: number,
    buzz: number = 123,
} = $props();

No more duplicate prop names, and the order of name-> type -> value is preserved.

brunnerh commented 11 months ago

Would it be possible for this to work with a type declaration directly on the variable rather than a generic? E.g.:

let someProp: string = $prop('default');

Generally not, that sort of annotation will only be checked for compatibility with the value being assigned, not change the type.

(Example)


@Snailedlt That should further simplify to:

let {
    foo = 'default',
    bar: number,
    buzz = 123,
} = $props();
aradalvand commented 11 months ago

@Snailedlt That syntax is already reserved by JavaScript for specifying aliases for the destructured properties. It can't be used for specifying types.

aradalvand commented 11 months ago

Actually, one problem with the singular $prop approach I proposed is that you won't be able to use reserved keywords as prop names β€” previously you could do that like so:

let classProp: string;
export { classProp as class };

And with the plural $props rune you could do it like this:

let { class: classProp } = $props();

But it doesn't seem like you'd be able to do the same thing with the singular $prop, as I proposed it; this, coupled with the fact the ...rest syntax won't work either (unless we were to also add a $restProps rune or something like that), makes me a little hesitant.

brunnerh commented 11 months ago

Edit: The following is now an error.


You can use $props multiple times, by the way.

let { foo = 'default' } = $props<{ foo: string }>();
let { bar } = $props<{ bar: number }>();
let { buzz = 123 } = $props<{ buzz: number }>();

Bit verbose, though.

aradalvand commented 11 months ago

@brunnerh But the prop names are still being repeated, which is the main problem.

Snailedlt commented 11 months ago

@Snailedlt That syntax is already reserved by JavaScript for specifying aliases for the destructured properties. It can't be used for specifying types.

@aradalvand Oh, didn't know that, how about this then?

let {
    foo<string> = 'default',
    bar<number>,
    buzz<number> = 123,
} = $props();
aradalvand commented 11 months ago

@Snailedlt Again, invalid syntax.

TheHadiAhmadi commented 11 months ago

What do you think about this syntax?

const klass = $prop<string>("class");
const htmlFor = $prop<string>("for");
const myNumber = $prop<number>("my-number"); // also kebab-case prop names would be possible

<MyComponent class="something" for="input-1" my-number={123} />
aradalvand commented 11 months ago

@TheHadiAhmadi Could work. We could also just use ?? [default] for specifying the default value.

let klass = $prop<string>('class') ?? 'default';

Update: I think ?? falls back to the right operand if the left operand is undefined or null, which is different from how default values in object destructuring work (e.g. let { foo = 'default' } = $props()), where it only falls back to the default value if the property is undefined (but not null). So, this idea may not be feasible either, given that.

Prinzhorn commented 11 months ago

I'm not even using TypeScript and would prefer having all props neatly separated. In the same way that I would never do let foo, bar; or export let foo, bar; because it is way less scannable (the eyes need to do more horizontal movement instead of just going down and scan over all declarations, yes I'm aware you can split the destructured object over multiple lines). Given that Svelte always follows the "code is read more often than it is written" mantra it is odd that the noise of destructuring was chosen over single props. But I understand that this is my personal preference and obviously we need to keep the $props rune for all kinds of reasons and edge cases.

I'll throw the following in the mix, why not (runes seem to be bending what's considered JavaScript anyway and declaring prop names as strings does not sit well with me):

const regularRequired = $prop<string>();
const reservedClassKeyword = $prop.class<string>();
const withDefault = $prop<number>(10);

But I kind of hate it, you can't immediately see the props this component receives, you need to pay attention to the .class. But I guess that's true for most suggestions, destructuring makes this the easiest due to proximity.

What if $prop also allows destructuring, but only to objects with a single property? These are compiler instructions after all, but we might anger the JavaScript police more and more as I speak:

const regularRequired = $prop<string>();
const { class: reservedClassKeyword } = $prop<string>();
const withDefault = $prop<number>(10);
dummdidumm commented 11 months ago

We did not add it yet for a couple of reasons:

look997 commented 11 months ago

This can be hidden in $props.single(). There is $effect.pre(), so it is similar....

WaltzingPenguin commented 11 months ago

Is it possible the priorities and trade offs decided are just wrong? Writing a TypeScript component is really common, exporting class is really uncommon (and I'd even argue an anti-pattern in 90% of cases).

Making the stuff I read and write every single time I open a file in a TypeScript project more difficult in exchange for making something I seldom do a little less kludgy (you still have to alias it after all), is a horrible trade off.

brunnerh commented 11 months ago

exporting class is really uncommon

It's really not, I do that all the time when wrapping base elements. Component libraries require this a lot unless they access $$props.

aradalvand commented 11 months ago

exporting class is really uncommon (and I'd even argue an anti-pattern in 90% of cases).

I strenuously disagree with both of those assertions β€” I would refer you to #6972.

MaximSagan commented 11 months ago

I'm not even using TypeScript and would prefer having all props neatly separated. In the same way that I would never do let foo, bar; or export let foo, bar; because it is way less scannable (the eyes need to do more horizontal movement instead of just going down and scan over all declarations, yes I'm aware you can split the destructured object over multiple lines). Given that Svelte always follows the "code is read more often than it is written" mantra it is odd that the noise of destructuring was chosen over single props. But I understand that this is my personal preference and obviously we need to keep the $props rune for all kinds of reasons and edge cases.

I'll throw the following in the mix, why not (runes seem to be bending what's considered JavaScript anyway and declaring prop names as strings does not sit well with me):

const regularRequired = $prop<string>();
const reservedClassKeyword = $prop.class<string>();
const withDefault = $prop<number>(10);

But I kind of hate it, you can't immediately see the props this component receives, you need to pay attention to the .class.

I feel like we're getting closer, but do we need to restrict ourself to only one "rune" for this task? As popular as reserved words may be in some cases (I agree "class" certainly is - and I don't think it should be considered an antipattern), for the most part they're still an edge case, so maybe better not to pollute the behavior of the default non-aliased $prop and just provide an additional "rune", e.g. something like $aliasedProp?

const regularRequired = $prop<string>();
const regularWithDefault = $prop<number>(10);
const aliasedRequired = $aliasedProp('class')<string>();
const aliasedWithDefault = $aliasedProp('catch')<boolean>(false);

An alternative signature for $aliasedProp could take two parameters, <T>(alias: string, defaultValue?: T) => T, but I slightly prefer the curried version I used in the example, (alias: string) => <T>(defaultValue?: T) => T, because with this version it's less likely the alias would be confused for the default value.

intrnl commented 11 months ago

Agree with having $aliasedProp or $prop.alias but highly disagree on the curry syntax as it looks busier with them.

Jak-Ch-ll commented 11 months ago

I feel like we're getting closer, but do we need to restrict ourself to only one "rune" for this task? As popular as reserved words may be in some cases (I agree "class" certainly is - and I don't think it should be considered an antipattern), for the most part they're still an edge case, so maybe better not to pollute the behavior of the default non-aliased $prop and just provide an additional "rune", e.g. something like $aliasedProp?

const regularRequired = $prop<string>();
const regularWithDefault = $prop<number>(10);
const aliasedRequired = $aliasedProp('class')<string>();
const aliasedWithDefault = $aliasedProp('catch')<boolean>(false);

An alternative signature for $aliasedProp could take two parameters, <T>(alias: string, defaultValue?: T) => T, but I slightly prefer the curried version I used in the example, (alias: string) => <T>(defaultValue?: T) => T, because with this version it's less likely the alias would be confused for the default value.

How about the following:

const regularRequired = $prop<string>();
const regularWithDefault = $prop(10);

const aliasedRequired = $prop<string>().as('class');
const aliasedWithDefault = $prop(false).as('catch');

Since the exposed property names for the component need to be statically analyzed anyway, we can do pretty much anything, as long as it's valid js syntax.

Jak-Ch-ll commented 11 months ago

When it comes to this discussion it's also probably worthwhile to check out the unofficial experimental defineProp macro for Vue: https://vue-macros.sxzz.moe/macros/define-prop.html

brunnerh commented 11 months ago

we can do pretty much anything, as long as it's valid js syntax.

That is not quite correct. In terms of types, runes expose the underlying values, so you cannot have chaining like that and still preserve the simple case without it.

E.g. $prop(10) will have the type number and thus no as function, so unless all types are polluted with those fake functions, that will cause issues. (Even then I was unable to get the typing to work as the this return type appears to not resolve correctly.)

Jak-Ch-ll commented 11 months ago

we can do pretty much anything, as long as it's valid js syntax.

That is not quite correct. In terms of types, runes expose the underlying values, so you cannot have chaining like that and still preserve the simple case without it.

E.g. $prop(10) will have the type number and thus no as function, so unless all types are polluted with those fake functions, that will cause issues. (Even then I was unable to get the typing to work as the this return type appears to not resolve correctly.)

Ah, you are right, I did not take the types into consideration. Then let me throw in another idea, which is a somewhat revised version of yours and takes additional influences from the Vue defineProp macro:

const aliasedRequired = $prop<string>(undefined, {
  name: 'class'
})
const aliasedWithDefault = $prop(false, {
  name: 'catch'
})

The main advantages are, that there is no need for an additional rune and, at least from my point of view, it's highly readable and intuitive what's going on.

The obvious disadvantage is that the syntax is somewhat more verbose, especially having to add undefined for required props. But as these renames should be the exception, I don't see this as a massive downside.

Another advantage is, that this syntax is extendable for future features, e.g. a runtime validation, similar to Vue

Not-Jayden commented 11 months ago

Ah, you are right, I did not take the types into consideration. Then let me throw in another idea, which is a somewhat revised version of yours and takes additional influences from the Vue defineProp macro:

const aliasedRequired = $prop<string>(undefined, {
  name: 'class'
})
const aliasedWithDefault = $prop(false, {
  name: 'catch'
})

One thing I'm not certain about with this syntax is, without Typescript, how is Svelte supposed to know that aliasRequired is required, or if it has just been defaulted to the value of undefined?

brunnerh commented 11 months ago

Good point regarding the default/required issue. We could maybe use a different syntax here, utilizing ?? πŸ€”

declare function $prop<T = undefined>(options?: any): T;

let required = $prop<number>();
let withDefault = $prop<number>() ?? 42;
let withDefaultImplicit = $prop() ?? 42;

let aliasedRequired = $prop<string>({ name: 'class '});
let aliasedWithDefault = $prop({ name: 'class '}) ?? 'flex';

// ?? removes undefined and null from the type
let requiredAllowUndefined = $prop<number | undefined>();
let requiredAllowNull = $prop<number | null>();
let withDefaultAllowUndefined = $prop() ?? (42 as number | undefined);
let withDefaultAllowNull = $prop() ?? (42 as number | null);

TS Playground

7nik commented 11 months ago
  1. ?? will replace both null and undefined with the default value, while the default value at restructuring replaces only undefined.
  2. The as operator does type casting with either type narrowing or extending and shouldn't be used at variable initialization unless you really want it. Example.
brunnerh commented 11 months ago

@7nik

  1. It would not need to behave that way, $prop() is not just a function either and the assigned variable becomes a signal; it all gets transformed anyway.
    Of course it might confuse people if a null value would override the default specified after ??.
  2. Right, it's either adding the type annotation to the variable, or the default value (using as). Setting the generic type parameter will not work here, though.

I was looking for a different syntax that might help express a default value and ?? seemed like the closest match that also kind of works with TS as is.

Maybe just a having a separate rune for the aliasing case is the best option if an additional argument does not work.
Always using an options argument might also be possible, but that would be a bit more verbose, i.e. something like:

let required = $prop<number>();
let withDefault = $prop({ default: 42 });
let aliasedRequired = $prop<string>({ name: 'class' });
let aliasedWithDefault = $prop({ name: 'class', default: 'flex' });
7nik commented 11 months ago

The main limitation is not making a js/ts correct code but making such syntax that any third-party js/ts parser/tool will understand the source code as correctly as possible (with, unfortunately, losing the understanding of reactivity and components interaction but nothing else).

This is why runes look like a function - thus, you can define them as a global function. But you cannot alter the behaviour of operators.

7nik commented 11 months ago

@brunnerh the last one probably is the best one, considering all the trade-offs and extendability. But I think specifying the type on the variable would be more natural:

let required: number = $prop();
let withDefault = $prop({ default: 42 });
let aliasedRequired: string = $prop({ name: 'class' });
let aliasedWithDefault = $prop({ name: 'class', default: 'flex' });
Jak-Ch-ll commented 11 months ago

@Not-Jayden What are your thoughts on using undefined to always make the prop required, while using null to explicitly set a non-value? Are there practical use-cases for which undefined as default value would be strictly necessary?

For me personally that fits into how I learned to think about the difference of null and undefined. If the developer wants to explicitly mark a value as non existent, they use null, while undefined means something was never there to begin with (or in this case we don't want to set anything as default value).

Not-Jayden commented 11 months ago

@Jak-Ch-ll For me it's less about the use case but the consistency of expectations. I'm not sure the difference between null/undefined is something that is super intuitive to translate to required/not required personally, but it could be fine.

Not-Jayden commented 11 months ago

@brunnerh the last one probably is the best one, considering all the trade-offs and extendability. But I think specifying the type on the variable would be more natural:

let required: number = $prop();
let withDefault = $prop({ default: 42 });
let aliasedRequired: string = $prop({ name: 'class' });
let aliasedWithDefault = $prop({ name: 'class', default: 'flex' });

Fifth option to put in the mix:

let className = $prop.aliased({name: 'class', default: 'flex'});

Not super stoked with that, but I think ideally the let foo = $prop('default'); syntax should try and be retained as it will be the most common use case, and it's the simplest syntax to support that use case.

Gin-Quin commented 9 months ago

You might be interested in this syntax that is valid JS and valid TS:

declare function $props<T = Record<string, never>>(): T;

const {
  myString = <string>"",
  myBoolean = <boolean>true,
  myOptionalBoolean = <boolean | undefined>undefined,
} = $props()

It follows nicely the "name -> type -> value" pattern without repetition.

But the issue is that it's impossible to know which props are required and which props are not.

7nik commented 9 months ago

@Gin-Quin, interesting approach. Probably props with undefined type should be optional.

Anyway, it won't allow to add descriptions to props:

/**
  * The user's name to greet
  */
export let name: string;

With $props it likely will be

type Props = {
  /**
    * The user's name to greet
    */
  name: string;
}

const { name } = $props<Props>();

But can you add sections to <string>""?

adiguba commented 9 months ago

Hello,

I support this idea too πŸ‘

I prefer to declare my props separately, and be able to type them concisely, and it's more consistent as it's close to the $state's syntax :

let a = $state(0);
let b = $prop(0);

IMHO, I think this could be done quite easily, given that internally $props is split into different function calls. Indeed if we take the following code with different use-cases :

let {
    // simple props
    optionalProp = 42,
    requiredProp,
    // props using another name :
    catch: theCatch,
    class: theClass = 'red',
    // rest props :
    ...everythingElse
} = $props();

Actually the $props() rune is compiled to distinct method call, like this :

let optionalProp = $.prop_source($$props, "optionalProp", 42, false),
    requiredProp = $.prop($$props, "requiredProp"),
    theCatch = $.prop($$props, "catch"),
    theClass = $.prop_source($$props, "class", 'red', false),
    everythingElse = $.rest_props($$props, [
        "optionalProp",
        "requiredProp",
        "catch",
        "class"
    ]);

I think we can use a similar pattern with 3 distincts runes : $prop(), $prop_as() and $rest_props().

In our example, this would result in the following code :

// simple props
let optionalProp = $prop(42);
let requiredProp = $prop();
// props using another name :
let theClass = $prop_as('class', 'red');
let theCatch = $prop_as('catch');
// rest props :
let everythingElse = $rest_props();

For TypeScript, these runes function can be defined like this :

declare function $prop<T>(): T|undefined;
declare function $prop<T>(defaultValue: T): T;

declare function $prop_as<T>(propName: string): T|undefined;
declare function $prop_as<T>(propName: string, defaultValue: T): T;

declare function $rest_props<T extends Record<string,unknown>>(): T;

So props with default value can be declared without declaring types :

// props with default values :
let optionalProp = $prop(42);  // optionalProp  is a number
let theClass = $prop_as('class', 'red'); // theClass is a string

Or we can specify it, for example to bring more constraint :

// props with default values :
let optionalProp : number = $prop(42);
let theClass : 'red'|'blue' = $prop_as('class', 'red');

Props without default values will be unknown by default, but we can specify the type, via Generic tag :

// props without default values :
let requiredProp = $prop<number>();  // requiredProp is a number|undefined
let theCatch = $prop_as<string>('catch'); // theCatch is a string|undefined

Or via the type definition (witch is equivalent) :

// props without default values :
let requiredProp : number | undefined = $prop(); 
let theCatch : string | undefined = $prop_as('catch');

The compiler should just check for potential duplicate props, like this :

let a = $prop(0);
let b = $prop_as('a', 1); // ERROR : duplicate prop "a"
adiguba commented 9 months ago

Note that $rest_props() could be replaced by **$props() if this issue is implemented : https://github.com/sveltejs/svelte/issues/9658

Snailedlt commented 9 months ago

I like the idea, but why would we want $prop_as to have a different name? Can't we just have an optional rename parameter to the $prop function?

So like this:

let number = $prop(0)
let renamedNumber = $prop('numberTwo', 0)
adiguba commented 9 months ago

@Snailedlt : because it can be ambiguous in some case. Ex this code :

let action = $prop('catch');

Is is

$prop() is going to be the most used and should be as clear and short as possible. Another specific case will be managed by specific runes... like $prop_as()

bfanger commented 8 months ago

A basic implemenation of the $prop rune https://github.com/bfanger/svelte/tree/feature/prop-rune

Usage:

let foo = $prop('default');
let bar: number = $prop();
let buzz = $prop(123);
brunnerh commented 8 months ago

@adiguba Two notes on your proposal:

Also want to emphasize that this really is an important issue for anyone using TypeScript. So even if this is tricky to get right on a technical level, it definitely would be very appreciated.

bfanger commented 8 months ago

@brunnerh The type definition was based on the $state rune, i've updated it to:

declare function $prop<T>(value?: T): T
Malix-Labs commented 8 months ago

Note #9764 Hoping it might be reverted in favor of the $prop rune in conjunction with #9948 behavior

Malix-Labs commented 8 months ago

Also, this issue should have the feature request and popular labels

Evertt commented 8 months ago

@bfanger how about this syntax for aliased props? I found that the word aliased is a little long for my taste, so I changed it to from.

declare const $prop: {
    <T>(defaultValue?: T): T
    from: {
        <T>(name: string, defaultValue?: T): T
    }
}

let count: number = $prop()

let countWithDefault = $prop(2)

let className: string = $prop.from("class")

let classNameWithDefault = $prop.from("class", "text-black")

TS playground

wwmike commented 8 months ago

Reading through this thread I like export let ... syntax more and more. Not sure why runes had to make this obsolete as well.

Malix-Labs commented 8 months ago

@Evertt ~Wouldn't count be number | undefined here?~ (see https://github.com/sveltejs/svelte/issues/9241#issuecomment-1879756897) ~Also,~ I think $prop.alias and $prop.as would make more sense and the latter be smaller than $prop.from

Evertt commented 8 months ago

@Malix-off nope, because count was not given a default value, it is therefor automatically marked as a required property. Which means you can define it as any type you want and the compiler will enforce that the consumer of the component MUST give that prop a value of that type.

garytube commented 8 months ago

Reading through this thread I like export let ... syntax more and more. Not sure why runes had to make this obsolete as well.

same. i hope they rethink this way

david-plugge commented 8 months ago

I don't get why you would prefer a this over $props. I always think of components like functions or classes, with arguments to call. Having a single "entrypoint" makes it very strict which is always a good thing. You need to take a look at the components props? Just look for $props! I feel like the new pattern is very strong, it brings nice typesafety, a very easy way to use restProps and even aliasing, which was annoyingly hard to achieve with export let.

wwmike commented 8 months ago

I don't get why you would prefer a this over $props. I always think of components like functions or classes, with arguments to call. Having a single "entrypoint" makes it very strict which is always a good thing. You need to take a look at the components props? Just look for $props! I feel like the new pattern is very strong, it brings nice typesafety, a very easy way to use restProps and even aliasing, which was annoyingly hard to achieve with export let.

Just read through this thread. I use typescript, and $props is everything, but elegant. It looks awful and hacky compared toexport let. If you consider component as a class, then you can consider export let ... as setters or public class variables. It exists in C#, Kotlin, many OO languages. You just define a property and it can be set from the outside. If you want a single entrypoint, nothing stops you from doing so, you can have a single property.

Also, is this really annoyingly hard?! export { _whatever as whatever }

Perhaps I'm wrong, but I see this change making my code unnecessarily ugly. As svelte compiles the code, I would love to see the problems solved while keeping the old syntax. From outside it's obviously easier to judge, but for me it seems like this solution is a "it works in other frameworks, just do that" kinda thing.