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

signal-based inputs and componentInputs type #464

Closed
xfh opened this issue Jun 26, 2024 · 5 comments · Fixed by #473
Closed

signal-based inputs and componentInputs type #464

xfh opened this issue Jun 26, 2024 · 5 comments · Fixed by #473

Comments

@xfh
Copy link

xfh commented Jun 26, 2024

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

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

@andreialecu
Copy link
Contributor

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

xfh commented Jul 29, 2024

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

andreialecu commented Jul 29, 2024

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants