ngneat / spectator

🦊 🚀 A Powerful Tool to Simplify Your Angular Tests
https://ngneat.github.io/spectator
MIT License
2.09k stars 177 forks source link

Angular 17 - input with transform #653

Open LukasMachetanz opened 7 months ago

LukasMachetanz commented 7 months ago

Is this a regression?

No

Description

Spectator does not correctly work using input having transform configured.

public id = input.required<number, string>({ transform: numberAttribute });

I would expect that Spectator correctly deals with this signature as well.

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

No response

Please provide the environment you discovered this bug in

No response

Anything else?

No response

Do you want to create a pull request?

No

LukasMachetanz commented 7 months ago

@kfrancois, I guess this could be connected to https://github.com/ngneat/spectator/issues/637.

kfrancois commented 7 months ago

@LukasMachetanz thank you for logging this issue!

Before we can work on this issue, could you provide some more information around the expected/current behaviour, or perhaps a reproduction?

I've tried the example you mentioned, and I'm not sure what should change around the current behaviour.

Note that this commit addressed input signals with transform: https://github.com/ngneat/spectator/commit/8cacddac18a2939fbf537bb74f0ca1dbcb1fa6e6#diff-3d128bff012b5140680c3a116bbeaceaa9721628c0e1ac71b672330a8b30dad0R11

In your example, input.required<number, string> would mean that if one were to pass an id into createComponent({ props: { id } }), the type of id is number | undefined, not string | number | undefined.

At the moment, this is a conscious choice (see discussion here https://github.com/ngneat/spectator/issues/649). If you believe this is wrong, we can re-open the discussion around this.

I hope this helps already!

LukasMachetanz commented 7 months ago

@kfrancois, thank you for the fast reply.


Let's assume having the following component:

import { ChangeDetectionStrategy, Component, input, numberAttribute } from "@angular/core";

@Component({
  selector: "pui-some",
  template: "SomeComponent",
  changeDetection: ChangeDetectionStrategy.OnPush,
  standalone: true
})
export class SomeComponent {
  public id = input.required<number, number | string>({ transform: numberAttribute });
}

And let's assume having the following test:

import { createComponentFactory } from "@ngneat/spectator";
import { SomeComponent } from "./some.component";

describe("SomeComponent", () => {
  const createComponent = createComponentFactory({
    component: SomeComponent
  });

  it("renders the component", () => {
    // TS2322: Type string is not assignable to type number
    const spectator = createComponent({ props: { id: "1" } });

    expect(spectator.component).toBeInstanceOf(SomeComponent);
  });
});

I would assume that it is possible to provide "1" as an input to id. Using the component within an template would allow <pui-some id="1" /> and <pui-some [id]="1" /> as well.

Therefore, I am wondering if I am understanding something wrong or if the behaviour within the test is really by intention?

Any help appreciated. :)

LukasMachetanz commented 6 months ago

@kfrancois, do you have any opinion on this?

kfrancois commented 6 months ago

@LukasMachetanz sorry for my slow response here - my personal opinion is that the current behaviour makes sense in most cases, as we'd want to pass in the type that the component actually uses (rather than the type that the transform function will accept).

My reasoning here is:

However, I understand that my opinion might not be universally applicable. As such, we could make the typing of InferInputSignal weaker to allow your proposal (basically by flipping <infer K, infer _> to <infer _, infer K>).

If we were to make this change, the drawback is that in the following example:

public id = input.required({ transform: numberAttribute });

The inferred type for id is InputSignalWithTransform<number, unknown> meaning that we'll accept unknown as an input for id. Personally, I think this is a worse API (see reasoning above) but if the general sentiment leans the other way, this can definitely be changed!

LukasMachetanz commented 6 months ago

@kfrancois, thank you for responding and sharing your opinion. I fear that I see this differently though. I would expect to test against the public interface of SomeComponent, hence to provide id as a number or as a string. A user of the mentioned component could do the same within the template after all. Additionally, I would not like to be forced to make the transform function public due to the test. In my opinion this is an implementation detail of the component itself.

Regarding the inferred type: In my opinion the inferred type should be the accepted type of the transform function. In the case of numberAttribute it would be unknown, but for a custom transform function it could be properly typed. Therefore this is no problem from my point of view. I would totally expect this.

LukasMachetanz commented 5 months ago

@kfrancois, do you have a final take on this? :)

kfrancois commented 5 months ago

Sorry @LukasMachetanz, I thought I responded to this already - I can agree with your reasoning, would you like to drop a PR which swaps the generic type for this?

I believe the change should just consist of this:

export type InferInputSignal<T> = T extends InputSignalWithTransform<infer _, infer K> ? K : T;

Thank you, and sorry again for taking so long to get back to you!

fredericojesus commented 2 months ago

I also have another issue that I'm not sure if others also have it. In both VSCode and Webstorm I get Object literal may only specify known properties, and 'profile' does not exist in type 'InferInputSignals<ProfileCompletionComponent>'. error.

CleanShot 2024-09-09 at 03 08 52

The code that I have works, but I get that annoying error.