Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Pass more strictNullChecks by changing onFinished in ComponentType to non-optional #10387

Closed
wants to merge 2 commits into from

Conversation

andybalaam
Copy link
Contributor

@andybalaam andybalaam commented Mar 15, 2023


This change is marked as an internal change (Task), so will not be included in the changelog.

@andybalaam andybalaam added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Mar 15, 2023
… non-optional

I believe this helps because the gymnastics with ComponentProps makes
onFinished doubly-optional, which then doesn't match the types of the
props of the dialog we are passing in to Modal.createDialog.

Reduces the number of strict errors by ~200 and does not introduce any
new ones.
@andybalaam andybalaam force-pushed the andybalaam/strictnullchecks-createDialog branch from 84e932a to 674eff1 Compare March 17, 2023 16:01
@andybalaam
Copy link
Contributor Author

@t3chguy would appreciate your thoughts on the second commit: 674eff1

I got to this point by working backwards from the types AsyncWrapper needed. WithOnFinished I think possibly represents the type we need, which is "anything so long as it has an onFinished method. Not very confident though, so would appreciate eyeballs.

@andybalaam
Copy link
Contributor Author

Actually, this does not work. We still need the ComponentProps mechanism - I'll look into reinstating that part.

Comment on lines 108 to +115
public createDialog<C extends ComponentType>(
Element: C,
props?: ComponentProps<C>,
props?: WithOnFinished,
className?: string,
isPriorityModal = false,
isStaticModal = false,
options: IOptions<C> = {},
): IHandle<C> {
options: IOptions = {},
): IHandle {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe these new changes solve element-hq/element-web#24899?

@andybalaam andybalaam closed this May 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants