Add support for input signals
kfrancois opened this issue ยท 14 comments
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
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.
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.
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)
I don't mind to introduce a breaking change. What do u have in mind?
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
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.
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!
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
@chocosd but in your example the original signal input is overridden, which may break the original default behaviour
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)
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!
@kfrancois should it also work having a transformer defined, hence having something like this:
public dashboardId = input.required<number, string>({ transform: numberAttribute });
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".
@kfrancois test fails if the input signal transforms the value to another type.