preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.81k stars 95 forks source link

The parameter of `singal()` can be omitted, just like `useState()`. #532

Closed cnbebop closed 5 months ago

cnbebop commented 8 months ago

Think about this, create a issue signal, and it's initial is undefined, then you should pass undefined to both generic parameter and function parameter.

const issue = signal<Issue | undefined>(undefined);

We can see that it leads to boilerplate code and the development experience is reduced.

The signal() can be defiened like this:

function signal<T>(value: T): Signal<T>;
function signal<T = undefined>(): Signal<T | undefined>;
XantreDev commented 8 months ago

I don't think it's a problem. Better to keep function with the same amount of arguments (it's better for performance)

cnbebop commented 8 months ago

I don't think it's a problem. Better to keep function with the same amount of arguments (it's better for performance)

Even if you're right, but the class Signal has an optional parameter constructor, how to explain it?

rschristian commented 8 months ago

Looks reasonable to me, if you want to PR it.

XantreDev commented 8 months ago

I don't think it's a problem. Better to keep function with the same amount of arguments (it's better for performance)

Even if you're right, but the class Signal has an optional parameter constructor, how to explain it? It's not for public usage. But I see, probably it will think that it can have 0 params https://github.com/preactjs/signals/blob/58befba577d02c5cac5292fda0a599f9708e908b/packages/core/src/index.ts#L531

Anyway I think it should be benchmarked. Check for deoptimizations and how much it slows other benchmarks

rschristian commented 8 months ago

Anyway I think it should be benchmarked. Check for deoptimizations and how much it slows other benchmarks

This should be a type signature-only change. There will be no effect on the benchmarks

JoviDeCroock commented 8 months ago

@XantreGodlike This is in relation to an already allowed paradigm, signals can be initiated with undefined or a missing initializer as you yourself link to for the Computed.prototype. This can't possibly affect benchmarks as it's merely a change in how the generic infers the starting value.

XantreDev commented 8 months ago

It's not we are starting to provide interface that allows different amount of args. It can cause performance slow down, depending on usage https://jsbench.me/7slty3mtij/1

image
XantreDev commented 8 months ago

But probably impact will be insignificant

JoviDeCroock commented 8 months ago

Did you read my comment? It's already ? allowed to be not there https://github.com/preactjs/signals/blob/main/packages/core/src/index.ts#L277

rschristian commented 8 months ago

It's not we are starting to provide interface that allows different amount of args

We are not. This usage has always been valid.

I don't think that benchmark is displaying what you think it is. Trying to do math with undefined is indeed going to be slower than adding two numbers together. This isn't going to be an issue here.

XantreDev commented 8 months ago

Did you read my comment? It's already ? allowed to be not there https://github.com/preactjs/signals/blob/main/packages/core/src/index.ts#L277

Got it. Sorry

kidonng commented 4 months ago

Woo-hoo, thanks for resolving this 🎉 Glad to see my PR #116 doesn't need to celebrate its 2nd birthday.