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

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

Closed
lacolaco opened this issue Jun 24, 2024 · 8 comments · Fixed by #465
Closed

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

lacolaco opened this issue Jun 24, 2024 · 8 comments · Fixed by #465

Comments

@lacolaco
Copy link
Contributor

lacolaco commented Jun 24, 2024

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

  • No any assertions in output testing scenario
  • Less and intuitive code to subscribe outputs

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');
});
  • Added outputs to RenderResult
  • outputs.get(name) can get a output ref by alias-aware name

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

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

mumenthalers commented Jul 8, 2024

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

mumenthalers commented Jul 9, 2024

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

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

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

@timdeschryver
Copy link
Member

timdeschryver commented Jul 11, 2024

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

@timdeschryver
Copy link
Member

@all-contributors please add @lacolaco for ideas

Copy link
Contributor

@timdeschryver

@lacolaco already contributed before to ideas

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