Skip to content

Request: Add template to RenderComponentOptions #203

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

Closed
lacolaco opened this issue Apr 20, 2021 · 6 comments · Fixed by #206
Closed

Request: Add template to RenderComponentOptions #203

lacolaco opened this issue Apr 20, 2021 · 6 comments · Fixed by #206
Labels

Comments

@lacolaco
Copy link
Contributor

lacolaco commented Apr 20, 2021

https://github.com/testing-library/angular-testing-library/blob/master/projects/testing-library/src/lib/models.ts#L266

TypeScript cannot determine which Type is a component or directive. Because template is from only RenderDirectiveOptions, TypeScript language service cannot suggest it for autocompletion.

In my opinion, render(Comp, { template }) is a common use-case, and it actually works well. I'm using render(Comp, { template } for my specs to use it as a living usage documentation at same time.

it('should accept [someFlag]', async () => {
  await render(FooComponent, {
    template: `<app-foo [someFlag]="true"></app-foo>`
  })
});
@timdeschryver
Copy link
Member

I agree!
It started out as a way to test directives, but I also use it more and more to test components.

The only "problem" is that the type won't be FooComponent in your example, because the component will be hosted in a host component.

I think the name of RenderDirectiveOptions is badly chosen from my end, but if I'm not mistaken, I think you would expect that the return type would be FooComponent and that you would get intellisense while providing component properties? I don't think that would be possible, or do you see a way to support this case?

@lacolaco
Copy link
Contributor Author

lacolaco commented Apr 21, 2021

@timdeschryver I can understand what you want to say, but I'm not expecting it (so far).
As you say, it is probably not intuitive that the target component is different from the host... Usually, I define the same props in the wrapper by using componentProperties.

const onSomeEvent = jest.fn();
const { rerender } = await render(FooComponent, {
  template: `<app-foo [someFlag]="someFlag" (someEvent)="onSomeEvent($event)"></app-foo>`,
  componentProperties: {
    someFlag: true,
    onSomeEvent,
  }
});
rerender({ someFlag: false });

I don't think this design is a mistake, but maybe not intuitive. My rough idea is adding render overload for the custom template use-case.

  • render(Type<T>, Options) -> render given component directly
  • render(string, Options) -> render wrapper component with given template.
const onSomeEvent = jest.fn();
await render(`<app-foo [someFlag]="someFlag" (someEvent)="onSomeEvent($event)"></app-foo>`, {
  declarations: [FooComponent],
  hostProperties: {
    someFlag: true,
    onSomeEvent,
  }
});

Honestly, I prefer separating two APIs like render and renderTemplate, but as the TestingLibrary family, render looks a special API.

How do you think?

@timdeschryver
Copy link
Member

timdeschryver commented Apr 21, 2021

I prefer to just keep it as render because of that's how we do it in TL (as you mentioned).

I really like the alternative version to render a template!
This would resolve the confusion between rendering a component and a host component.
If you want you can make these changes and open a PR, otherwise I will probably implement it somewhere next week.

@lacolaco
Copy link
Contributor Author

I'd like to create a PR as my first contribution for testing-library! 👍 I'll work on it in a few days.

@timdeschryver
Copy link
Member

Awesome, thanks @lacolaco !
If there's anything, feel free to ping me :)

@github-actions
Copy link

🎉 This issue has been resolved in version 10.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

2 participants