Skip to content

Change render signature to function from property#2397

Merged
timroes merged 1 commit intoelastic:masterfrom
timroes:prepare-ts-3.7
Oct 4, 2019
Merged

Change render signature to function from property#2397
timroes merged 1 commit intoelastic:masterfrom
timroes:prepare-ts-3.7

Conversation

@timroes
Copy link
Copy Markdown
Contributor

@timroes timroes commented Oct 3, 2019

Summary

I am currently preparing the TS 3.7 upgrade in Kibana (elastic/kibana#47188). There is currently one failure coming from EUI, that this PR fixes. It seems that newer TS versions make a difference between having a function (that render is in the base class) and an property assigned an arrow function, which we had in this typing. So TS 3.7 will fail on this on:

node_modules/@elastic/eui/eui.d.ts(2417,6): error TS2424: Class 'Component<CommonProps & ButtonHTMLAttributes<HTMLButtonElement> & EuiFilterSelectItemProps, {}, any>' defines instance member function 'render', but extended class 'EuiFilterSelectItem' defines it as instance member property.

Changing the typing here slightly (which should not have any other effect) prevents this error from happening. Since this should have no effect (except that you later might work with TS 3.7.0) I didn't add a changelog for that.

Note: There are a couple of more issues if you want to actually upgrade EUI to TS 3.7.0, that will need to be fixed, but this is the only one actually preventing consumers to upgrade to 3.7.0 later on, so I wanted to focus on this for now.

Checklist

  • [ ] Checked in dark mode
  • [ ] Checked in mobile
  • [ ] Checked in IE11 and Firefox
  • [ ] Props have proper autodocs
  • [ ] Added documentation examples
  • [ ] Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • [ ] A changelog entry exists and is marked appropriately

Copy link
Copy Markdown
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@chandlerprall I couldn't find an EUI issue for tracking an eventual TS upgrade. Do you know of one/should we get one going?

@timroes timroes merged commit c5bf02f into elastic:master Oct 4, 2019
@timroes timroes deleted the prepare-ts-3.7 branch October 4, 2019 07:35
@chandlerprall
Copy link
Copy Markdown
Contributor

@thompsongl no, we do not have one yet

snide pushed a commit to snide/eui that referenced this pull request Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants