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

Add support for input signals #637

Closed
kfrancois opened this issue Jan 30, 2024 · 14 comments
Closed

Add support for input signals #637

kfrancois opened this issue Jan 30, 2024 · 14 comments

Comments

@kfrancois
Copy link
Contributor

kfrancois commented Jan 30, 2024

Description

Angular 17.1 introduced input signals, which aren't supported yet by a few tools as the feature is quite new.

I've done some initial investigation in how to address this in @ngneat/spectator (see Proposed solution below) but mainly want to know what the maintainers' plans are when it comes to supporting input signals, and how I could help out.

We use Spectator pretty heavily and as testing is important, I'd love to be able to use Spectator for components that use input signals 😄

Proposed solution

A simple component using input signals could look like this:

@Component({
  selector: 'app-signal-input',
  template: `
    @if(show()) {
      <div id="text">Hello</div>
    }
  `,
  standalone: true,
})
export class SignalInputComponent {
  public show = input(false);
}

This affects Spectator in two ways:

1. The type of inputs now needs to be inferred when the input is an input signal

When testing the above component, we want to do something like this:

const spectator = createComponent({ props: { show: true } });

Because Spectator does not know about input signals yet, Typescript will throw an error: Type 'boolean' is not assignable to type 'InputSignal<boolean, boolean>'.

Fixing this is pretty simple, we can create a type like this to conditionally infer a signal's type:

export type InferSignalInputs<C> = {
  [P in keyof C]+?: C[P] extends InputSignal<infer T> ? T : C[P];
};

2. Setting input signals is not the same as setting regular inputs

I'm not quite sure how to handle this as I haven't dug into the Angular source code enough to know how signals/input signals work under the hood.

Spectator's setProps function currently sets the following when changing inputs/props:

instance[key] = newValue;

I'm not sure whether a simple check like this would suffice:

if (isSignal(instance[key])) {
  instance[key] = signal(newValue);
} else {
  instance[key] = newValue;
}

I'm assuming the above code will not work as intended in all cases. This is where I'm looking for some feedback/help.

Alternatives considered

  • Using TestBed for the time being (manually setting fixture.componentInstance.show somehow)

Do you want to create a pull request?

Yes

@NetanelBasal
Copy link
Member

We need to change the internal setInput to use the component ref setInput method and everything should work. Regarding the types, we can improve it. You're welcome to create a PR.

@kfrancois
Copy link
Contributor Author

Thanks for your response @NetanelBasal!

I believe I can get something (mostly) ready but will be blocked by thymikee/jest-preset-angular#2255 as jest-preset-angular does not support (see angular/angular#54013).

One thing setInput does not address is createHost/createDirective usages.
With these two factories we have access to the ComponentRef of the host component, but we don't have a ComponentRef of the component that's being tested.

As setInput is only available on a ComponentRef, I don't think we're able to use it for these factories.

I'd love an implementation like this:

export function setProps(componentRef: ComponentRef<T>, keyOrKeyValues: any, value?: any): any {
  if (isString(keyOrKeyValues)) {
    componentRef.setInput(keyOrKeyValues, value);
  } else {
    // eslint-disable-next-line guard-for-in
    for (const p in keyOrKeyValues) {
      componentRef.setInput(p, keyOrKeyValues[p]);
    }
  }

  return componentRef.instance;
}

But this would necessitate changes in the createHost/createDirective API's as setProps would accept ComponentRef<T> rather than T, and createHost/createDirective are testing a child component/directive where we have access to the class instance but not the `ComponentRef.

@kfrancois
Copy link
Contributor Author

I feel like the createHost/createDirective API's can be a bit confusing at the moment as it's possible to set inputs through either te template or through props, perhaps this can be an opportunity to change this behaviour (albeit with a deprecation/breaking change)

@NetanelBasal
Copy link
Member

I don't mind to introduce a breaking change. What do u have in mind?

@kfrancois
Copy link
Contributor Author

kfrancois commented Feb 1, 2024

At the moment, we can set inputs in two ways with createHost/createDirective: through the template (<div> <my-component [show]="true" /> </div>) or through setInput (spectator.setInput({ show: true })).

This could be confusing, as I can do the following in code:

const spectator = createHost(`<div> <my-component [show]="show" /> </div>`);

// Two ways to set the same input, could be confusing
spectator.setHostProps({ show: false });
spectator.setProps({ show: true });

I propose removing setProps here, and forcing the developer to use the template instead. The biggest drawback here is that we have to use the template and can no longer pass in { props: defaultProps } in every test.

Ideally this type of breaking change is something we want to avoid, but I'm not sure how to keep the API intact and support input signals for createHost/createDirective

@NetanelBasal
Copy link
Member

I agree that it's not the normal flow to call spectator.setProps({ show: true }) when you use host. We should update the host inputs which should trigger cd on the child when needed.

@kfrancois
Copy link
Contributor Author

I added this functionality in #638 which works (all tests are passing locally) but there's some cleanup needed in related types + docs. Let me know what you think & I can take these up!

@chocosd
Copy link

chocosd commented Feb 19, 2024

sorry to but in, but you can already test signalInputs ? I've written some tests the other day with them, and currently you can just do something like:

  describe('text', () => {
    it('should use the text input for the anchors text content', () => {
      const text = input('our link text');

      spectator.setInput('text', text);
      spectator.detectChanges();

      expect(spectator.query('a')).toHaveText(text());
    });
  });

and it seems to be fine

@twittwer
Copy link
Contributor

@chocosd but in your example the original signal input is overridden, which may break the original default behaviour

@kfrancois
Copy link
Contributor Author

What @twittwer said indeed 😄 the changes from PR basically allow us to use Angular's own componentRef.setInput() method.

This allows us to stay a bit closer to TestBed or Angular's actual runtime behaviour, instead of manually setting fields/running changedetection (which is more brittle).

So while technically assigning a new InputSignal to the component instance works in Spectator, this is not how components behave at runtime. Kinda similar to how we want to set new values on a subject this.behaviorSubject$.next(value) instead of resassigning this.behaviorSubject$ = new BehaviorSubject(value)

@kfrancois
Copy link
Contributor Author

I'll close this atm as #638 got merged - if there happen to be any issues with these changes, or Signal/InputSignal support let me know!

@LukasMachetanz
Copy link

@kfrancois should it also work having a transformer defined, hence having something like this:

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

@LukasMachetanz
Copy link

LukasMachetanz commented Feb 22, 2024

I am also experiencing problems for:

 const spectator = createDirective("<ng-container platformUiEditorShell [data]='data'></ng-container>", { hostProps: { data: "My Data" } });

It seems that this problem only occurs having the input named "data".

@adlaonmd
Copy link

@kfrancois test fails if the input signal transforms the value to another type.

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

No branches or pull requests

6 participants