Skip to content

Conversation

@Markionium
Copy link
Member

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

BaseComponent defines the componentRef as receiving null as a possible value.

componentRef?: (ref: T | null) => (void | T);

However a lot of the components defined the prop without taking the null value into account. But do not guarantee that this is actually the case in the component. This caused a hole in the type system as the function can be called with null but we don't protect against it.

componentRef?: (component: ICommandBar) => void;

This PR fixes up all the componentRef type definitions by adding the possible null value.

componentRef?: (component: ICommandBar | null) => void;

Focus areas to test

Effects types only, so should not have any effect on JS.

@Markionium Markionium mentioned this pull request Mar 22, 2018
2 tasks
@jordandrako
Copy link
Contributor

jordandrako commented Mar 22, 2018

@Jahnp Can you manually approve/merge this? npm start is borked at the moment and this will should fix it.

@Jahnp
Copy link
Member

Jahnp commented Mar 22, 2018

@jordandrako, it looks good to me, but with a change this wide-ranging, @dzearing should really be the one to merge.

@dzearing dzearing merged commit a0e0969 into microsoft:master Mar 23, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants