hilla icon indicating copy to clipboard operation
hilla copied to clipboard

[Full-stack Signals] allow client-side signal's default value to be set upon creation

Open taefi opened this issue 1 year ago • 11 comments

Describe your motivation

As prerequisite for offline-support, it is necessary for the client-side instance of a shared signal to have a default value when created, without having to wait for a round-trip to the server (to bring the actual default / latest value).

Describe the solution you'd like

It is suggested to have an API such as withDefaultValue for signals that allows the default value to be set:

const counter = CounterService.counter().withDefaultValue(1);
const name = NameService.name().withDefaultValue("");
const approved = LicenseService.approved().withDefaultValue(false);

If the withDefaultValue is not used, the default values will be determined as follows:

  • NumberSignal and ValueSignal<number> => 0
  • ValueSignal<boolean> => false
  • ValueSignal<string> => undefined

Describe alternatives you've considered

No response

Additional context

No response

taefi avatar Jul 25 '24 12:07 taefi

I suspect it would be confusing to have different default initial values for different T types in ValueSignal<T> since that type information only exists in TS but not in the transpiled JS. There's nothing that prevents us from embedding the information when we generate the TS code but it might still be confusing to the developer. It would also open up a can of worms with regards to whether the default for e.g. ValueSignal<string> should be undefined or an empty string.

A related note is that the inability of knowing the initial value means that the type of NameService.name() would have to be ValueSignal<string | undefined> whereas the type of NameService.name().withDefaultValue("") would be ValueSignal<string>. This does on the other hand lead to the potentially unwanted side effect that it would be valid to do someValueSignal.value = undefined unless we do some crazy things with the TS types.

Legioth avatar Jul 29 '24 10:07 Legioth

In this case, do you see any caveats in implementing #2623 in a way that keeps ValueSignal<T> abstract as it is and introduces StringSignal and BooleanSignal? Then, for implementing this API, each concrete type would implement its own typed version of the withDefaultValue to remove the confusion for developers.

Deciding on the default value of StringSignal to be either undefined or an "empty string" is a hard thing ;) Personally, I'm found of the idea of a more limited version for implementation without supporting undefined at all for the so called "StringSignal".

taefi avatar Jul 29 '24 12:07 taefi

What value would a dedicated StringSignal or BooleanSignal type have other than using a different default value?

I was also thinking that we could provide an annotation for defining the default value on a method with the gotcha that this value would always be a (JSON) String due to the way annotations work. In that way, the default could be embedded in the generated TS and thus be immediately available on the client.

@DefaultValue("false")
public ValueSignal<Boolean> approved() {
  return approved;
}

Legioth avatar Jul 30 '24 07:07 Legioth

keeps ValueSignal<T> abstract as it is and introduces StringSignal and BooleanSignal?

What about ValueSignal<LocalDate>, ValueSignal<MyBean> or any other type that you might want to send as a readonly JSON object?

Legioth avatar Jul 30 '24 07:07 Legioth

To make sure I'm understanding what are we trying to solve here, I need to understand "what" and "why" having something like the following could be confusing for the developers:

When implementing #2623, we can expose ValueSignal<T> as a non-abstract class to hold any generic values (that we can support in Hilla), so an instance of ValueSignal<T> would only accept type default values of type T as args. In this way the instances are obtained from the service are typed like this (the following service wrapper would be generated in the future):

export class ValueSignalServiceWrapper {
  static someBooleanValue() {
    const signalChannel =
      new ValueSignalChannel<boolean>('ValueSignalService.someBoolean', client_1);
    return signalChannel.signal;
  }

  static someStringValue() {
    const signalChannel =
      new ValueSignalChannel<string>('ValueSignalService.someString', client_1);
    return signalChannel.signal;
  }

  static someDateValue() {
      const signalChannel =
      new ValueSignalChannel<Date>('ValueSignalService.someDate', client_1);
      return signalChannel.signal;
  }
}

Then, both the compiler and the IDE are accepting/suggesting the correct type on each instance: Screenshot 2024-07-30 at 13 37 48

Screenshot 2024-07-30 at 13 38 12

Screenshot 2024-07-30 at 13 39 05

Here, I'm only talking about the shape of client-side API (at the current stage of developing signals). When we start implementing the proper generation of endpoints (#2628), probably additional meta-data are needed for boosting the generator with correct type and defaults being propagated.

taefi avatar Jul 30 '24 10:07 taefi

What's the type before the withDefaultValue() step, i.e. the type of x in this example:

const x = ValueSignalServiceWrapper.someBooleanValue();

I would recommend typing out the return type in the generated code rather than letting it be inferred. This helps make the code slightly more readable and makes it slightly easier to detect any unexpected bugs in the code generation.

Legioth avatar Jul 31 '24 06:07 Legioth

The type inference with ValueSignal<T> seems to be working fine: Screenshot 2024-07-31 at 10 44 50 And strictly typing them is also fine: Screenshot 2024-07-31 at 10 47 36 So nothing prevents us from adding the types in the generated service codes as well, though, the above is also fine IMO.

taefi avatar Jul 31 '24 07:07 taefi

Should it be ValueSignal<boolean> or ValueSignal<boolean | undefined> before withDefaultValue? booleanValue.value will be undefined before the initial value has been fetched from the server which means that users might observe a value that isn't compatible with the declared type if that's ValueSignal<boolean>.

Legioth avatar Jul 31 '24 07:07 Legioth

My suggestion about typing out the types was for the method signature in the generated code, e.g.

static someBooleanValue(): ValueSignal<boolean | undefined> {

Legioth avatar Jul 31 '24 07:07 Legioth

booleanValue.value will be undefined before the initial value has been fetched from the server ...

This is missing from the typing at the moment. The ValueSignal<T> should be able to support having that undefined state, and the NumberSignal should inherit it (or change to support this).

taefi avatar Jul 31 '24 08:07 taefi

NumberSignal has 0 as the default value so there's no need for undefined there.

Legioth avatar Jul 31 '24 08:07 Legioth

As discussed separately, the use of an instance method messes up type signatures especially in combination with nullability since there are cases where the setter and the getter would have different types.

Instead, we should provide a way of defining the default value through an options object when creating an instance, i.e. NameService.name({defaultValue: ""});

Legioth avatar Sep 09 '24 11:09 Legioth

With endpoint arguments:

const signal: ValueSignal<string> = SomeService.someSignal(someArgument, {defaultValue: "foo"});

platosha avatar Sep 18 '24 10:09 platosha