testing-library / angular-testing-library

🐙 Simple and complete Angular testing utilities that encourage good testing practices
https://testing-library.com/angular
MIT License
709 stars 90 forks source link

signal-based inputs and componentInputs type #464

Closed xfh closed 1 month ago

xfh commented 2 months ago

Hi

I started migrating some components to signal based input and model functions. Unfortunately, that causes typing issues. RenderComponentOptions declares componentInputs?: Partial<ComponentType> | { [alias: string]: unknown };, which resolves the types as InputSignal or similar.

Would you accept a helper type to unwrap the value of signals? I think this should do the trick:

type ComponentInput<T> = {
    [P in keyof T]?: T[P] extends Signal<infer U>
        ? Partial<U> // If the property is a Signal, apply Partial to the inner type
        : Partial<T[P]>; // Otherwise, apply Partial to the property itself
};

This doesn't help much when using the render function directly, because of the type union for the aliases. But when using a setup function we can declare an input property without the aliases and benefit from better type support. E.g.

function setup(
    componentInputs: ComponentInput<PagesComponent>,
): Promise<RenderResult<PagesComponent>> {
    return render(PagesComponent, {
        componentInputs
    });
}

Obviously, I can do all this without a change in angular-testing-library. I just thought it might become a common issue with growing adoption of signal inputs and wanted to share my workaround.

timdeschryver commented 1 month ago

@xfh sorry for my late response. Feel free to open a PR for this.

andreialecu commented 1 month ago

I initially ran into this because I was using componentProperties instead of componentInputs in tests. I found this issue and started working on a PR without realizing that we were using the wrong field. After switching to the latter the type errors went away.

This is because of { [alias: string]: unknown } in the type, which essentially disables type checking on it.

I wonder what type errors OP was running into.

xfh commented 1 month ago

Hi, sorry for the late replay - I'm on holidays.

@andreialecu, I typically use a setup function for my component tests, where I used to type the inputs as Partialy<ComponentType>. Note that I am not using the type union with { [alias: string]: unknown } (because I usually don't use aliases and I prefer correctly typed inputs). With signal-based inputs that approach fails: the input gets resolved to an InputSignal. My little type helper provides correct types for signal based inputs. You are correct though, that the render function and the componentInputs already allow passing values to signal-based inputs.

In my opinion, I'd be good to provide better types for componentInputs and remove the type union. That would be a breaking change though. I am assuming, that the type union was introduced to support input with an alias? We could provide another api to support aliases.

What do you think @timdeschryver ?

andreialecu commented 1 month ago

I'd like to add that the union with { [alias: string]: unknown } is essentially like typing it any. Seems adding the "alias" option this way wasn't the best idea :).

One fix would be to deprecate the property and create a brand new one which allows aliases via some branded type.

For example:

inputs: { foo: 123, aliased: aliasedInputWithValue('someName') }

Where aliasedInputWithValue is an utility function that returns some sort of AliasedInput<T> type, for type safety purposes. Then inputs would be ComponentInput<T> | AliasedInput.

Then it's not necessarily a breaking change, the old property will continue to work as is.

andreialecu commented 1 month ago

I opened a PR at #473 with my proposal. Consider it a draft for now.