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

Add stronger types #413

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Christian24
Copy link

Closes #412

@@ -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!

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 this pull request may close these issues.

Better types for RenderComponentOptions
3 participants