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!

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Since we're providing better types for inputs and outputs, I kind of want to go forward with this change. Even if the Angular team does not provide it out of the box.
As this is a breaking change, I'll keep it open until our next major release.

What do you think @Christian24 ?

@Christian24
Copy link
Author

Sounds good to me. Do you want me to resolve conflicts or should be just leave it for the next major release?

@timdeschryver
Copy link
Member

timdeschryver commented Aug 15, 2024

You can leave it as is for now @Christian24 , thanks!

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