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.

Sign up for GitHub

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 stronger types #413

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+15 −5
Open

Add stronger types #413

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b880be9
Add stronger types
Sep 11, 2023
04c9711
Fix review feedback
Christian24 Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions projects/testing-library/src/lib/models.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Type, DebugElement } from '@angular/core';
import { Type, DebugElement, ModuleWithProviders, EnvironmentProviders, TypeProvider } from '@angular/core';
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { Routes } from '@angular/router';
import { BoundFunction, Queries, queries, Config as dtlConfig, PrettyDOMOptions } from '@testing-library/dom';
Expand Down Expand Up @@ -65,6 +65,12 @@ export interface RenderResult<ComponentType, WrapperType = ComponentType> extend
) => Promise<void>;
}

export interface ProviderInterface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for creating this PR.

The reason why this is typed as any is because it's also like that within the Angular TestBed. Let me think on this for a while longer before deciding to merge this in.

Do you think this could potentially break something?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. Take your time. I know this is different from the Implementation in Angular and I have been thinking about raising an issue with them as well.

So with all the types I looked at Angular proper (not the Testbed) and adapted accordingly. The imports type is straight from Angular proper.

The providers types I basically looked at what does Angular proper have as type definition and made it a bit more loose. There is a Provider type in Angular, but intentionally I only tried to mimic the structure and not the types. useValue and provide are still any. I basically want to make it easier for people writing tests. Because as it is now you get no auto completion or even error if you make a mistake.

That said I do not know if this could potentially break something.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened an issue in the angular repo: angular/angular#51765

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently the Angular team is already at it: angular/angular#37178

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Christian24 , because the Angular team is busy with it I want to wait until they provide the proper types for it. Otherwise it could lead into breaking changes or other things.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I subscribed myself to the PR. I will update mine as soon as it is merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan!

provide: any;
useValue: any;
multi?: boolean;
}

export interface RenderComponentOptions<ComponentType, Q extends Queries = typeof queries> {
/**
* @description
Expand Down Expand Up @@ -128,7 +134,7 @@ export interface RenderComponentOptions<ComponentType, Q extends Queries = typeo
* ]
* })
*/
providers?: any[];
providers?: Array<ProviderInterface | TypeProvider | EnvironmentProviders>;
/**
* @description
* A collection of imports needed to render the component, for example, shared modules.
Expand All @@ -147,7 +153,7 @@ export interface RenderComponentOptions<ComponentType, Q extends Queries = typeo
* ]
* })
*/
imports?: any[];
imports?: Array<Type<any> | ModuleWithProviders<unknown> | any[]>;
/**
* @description
* A collection of schemas needed to render the component.
Expand Down Expand Up @@ -231,7 +237,7 @@ export interface RenderComponentOptions<ComponentType, Q extends Queries = typeo
* ]
* })
*/
componentProviders?: any[];
componentProviders?: Array<ProviderInterface | TypeProvider | EnvironmentProviders>;
Christian24 marked this conversation as resolved.
Show resolved Hide resolved
/**
* @description
* Collection of child component specified providers to override with
Expand Down