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

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

Open
cnbebop opened this issue Mar 18, 2024 · 11 comments
Open

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

cnbebop opened this issue Mar 18, 2024 · 11 comments
Labels
good first issue Good for newcomers types

Comments

@cnbebop
Copy link

cnbebop commented Mar 18, 2024

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
Copy link
Contributor

XantreDev commented Mar 18, 2024

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

@cnbebop
Copy link
Author

cnbebop commented Mar 19, 2024

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

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

@rschristian rschristian added the good first issue Good for newcomers label Mar 19, 2024
@XantreDev
Copy link
Contributor

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

Computed.prototype = new Signal() as Computed;

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

@rschristian
Copy link
Member

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

@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
Copy link
Contributor

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
Copy link
Contributor

But probably impact will be insignificant

@JoviDeCroock
Copy link
Member

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

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers types
Projects
None yet
Development

No branches or pull requests

4 participants