-
Notifications
You must be signed in to change notification settings - Fork 90
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
base: main
Are you sure you want to change the base?
Add stronger types #413
Conversation
@@ -65,6 +65,12 @@ export interface RenderResult<ComponentType, WrapperType = ComponentType> extend | |||
) => Promise<void>; | |||
} | |||
|
|||
export interface ProviderInterface { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a plan!
There was a problem hiding this 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 ?
Sounds good to me. Do you want me to resolve conflicts or should be just leave it for the next major release? |
You can leave it as is for now @Christian24 , thanks! |
Closes #412