testing-library / angular-testing-library

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

Proposal: new way to spy output without `any` assertion #462

Closed lacolaco closed 4 months ago

lacolaco commented 5 months ago

Context

I think that there is some pains in spying on the output of a component, which is typified by the fact that you have to use type assertions by any. It is my opinion that the cost to pay for simply subscribing to events emitted by a component and inspecting their values can be lesser.

https://github.com/testing-library/angular-testing-library/blob/main/apps/example-app/src/app/examples/22-signal-inputs.component.spec.ts#L51-L66

test('output emits a value', async () => {
  const submitFn = jest.fn();
  await render(SignalInputComponent, {
    componentInputs: {
      greeting: 'Hello',
      name: 'world',
    },
    componentOutputs: {
      submit: { emit: submitFn } as any,  // <==
    },
  });

  await userEvent.click(screen.getByRole('button'));

  expect(submitFn).toHaveBeenCalledWith('world');
});

Goals

Idea

This is a pseudo code and has not been strictly validated for feasibility.

test('output emits a value', async () => {
  const submitFn = jest.fn();
  const { outputs } = await render(SignalInputComponent, {
    componentInputs: {
      greeting: 'Hello',
      name: 'world',
    },
  });
  outputs.get('submit').subscribe(submitFn);

  await userEvent.click(screen.getByRole('button'));

  expect(submitFn).toHaveBeenCalledWith('world');
});

outputs.get(eventName)

The eventName should be same to its name in HTML template, <some-cmp (eventName)="..." />. Angular's ComponentMirror has outputs getter which returns a record of propName and tempalteName of outputs. So I think ALT can map the eventName to corresponding property.

https://github.com/angular/angular/blob/main/packages/core/src/render3/component.ts#L104-L139

// pseudo code to get OutputRef from eventName
function getOutputRef<C, T>(componentRef: ComponentRef<C>, eventName: string): OutputRef<T> {
  const { outputs } = reflectComponentType(componentRef.componentType);
  const output = outputs.find(o => o.propName === eventName);
  if (output == null) {
    throw new Error(`Output ${eventName} doesn't exist.`);
  }
  return componentRef.instance[output.templateName];
}
timdeschryver commented 5 months ago

Thanks for the input @lacolaco ! I agree that the current design works, but can be improved. I was thinking of introducing a new createOutputListener method as a work around.

test('output emits a value', async () => {
  const submitFn = jest.fn();
  const { outputs } = await render(SignalInputComponent, {
    componentInputs: {
      greeting: 'Hello',
      name: 'world',
    },
    componentOutputs: {
      submit: createOutputListener(submitFn)
    }
  });

  await userEvent.click(screen.getByRole('button'));

  expect(submitFn).toHaveBeenCalledWith('world');
});

That being said, I like your approach as well, and I think it will work better with Angular in the future (if something will be changed for Output properties).

Let's think on this for a while - I'll also send a tweet of other opinions (feel free to do this as well).

mumenthalers commented 4 months ago

@timdeschryver unfortunately the current approach of overriding the output property is faulty for the case when the output is not a EventEmitter but an actual Observable:

@Component({...})
class MyComponent {
  @Output() readonly myEvent = fromEvent<MouseEvent>(inject(ElementRef).nativeElement, 'click')
}

Overriding myEvent has the effect of breaking the implementation.

Although it was never intended like this, it always worked and eventually became official when angular introduced the outputFromObservable (rxjs-interop) with the new functional output api.

imo the approach of simply overriding is questionable anyway as compared to componentInputs which actually sets inputs with angulars setInput method.

Therefore an implementation like @lacolaco should be considered.

or a type-safe approach could be preferred instead (and simply ignoring the output aliases):

  function subscribeToOutput<T>(
    componentRef: ComponentRef<T>,
    key: { [key in keyof T]: T[key] extends OutputRef<any> ? key : never }[keyof T],
    spy: (value: any) => void,
  ) {
    const unsubscribe = (componentRef.instance[key] as any).subscribe(spy)
    componentRef.onDestroy(unsubscribe)
    return unsubscribe
  }

(subscribeToOutput could be returned from the render function with bound componentRef)

mumenthalers commented 4 months ago

@timdeschryver after digging into the code and thinking about it, I would propose an api like this:

interface RenderComponentOptions<ComponentType> {
  ...
  /**
   * @description
   * An object with key-value pairs to subscribe to EventEmitters of the component where `key` is the actual property name (no alias support) and value the callback function to subscribe with.
   *
   * @default
   * {}
   *
   * @example
   * const sendValue = (value) => { ... }
   * await render(AppComponent, {
   *  subscribeToOutputs: {
   *    send: (_v:any) => void
   *  }
   * })
   */
  subscribeToOutputs?: {
    [key in keyof ComponentType as ComponentType[key] extends OutputRef<any> ? key : never]?: ComponentType[key] extends OutputRef<infer U> ? (val: U) => void : never
  };
  ...
}

The componentOutputs should then become deprecated in favour of this new subscribeToOutputs api.

What do you think? Would be happy to create a PR --> proposed implementation

timdeschryver commented 4 months ago

Thanks for the input and the effort @mumenthalers . I like the proposed API and the reasoning behind it.

My initial thought is to merge these changes in ATL. @lacolaco since you proposed this new API, could you also share your thoughts about this please?

lacolaco commented 4 months ago

@timdeschryver Totally I like @mumenthalers 's subscribeToOutputs because it is more declarative than my proposed idea!

timdeschryver commented 4 months ago

@mumenthalers feel free to create a PR, we can further discuss it there (if needed) :)

timdeschryver commented 4 months ago

@all-contributors please add @lacolaco for ideas

allcontributors[bot] commented 4 months ago

@timdeschryver

@lacolaco already contributed before to ideas