Upgrade Typescript to 4.5.3 to match Kibana #5591

cee-chen · 2022-02-02T21:09:12Z

Summary

Kibana is currently pinned at Typescript 4.5.3 (elastic/kibana#120812), so we should upgrade EUI to match (which also allows us to upgrade react-docgen-typescript to 2.x+, as their 2.x+ requires typescript 4.3+).

Checklist

Props have proper autodocs and playground toggles
Checked for breaking changes and labeled appropriately
A changelog entry exists and is marked appropriately

- to match Kibana

Property 'MSInputMethodContext' does not exist on type 'Window & typeof globalThis'

Error: 'onClick' is specified more than once, so this usage will be overwritten.

Errors: - Initializer provides no value for this binding element and the binding element has no default value. - Binding element '_onClick' implicitly has an 'any' type. Fixes: - Fix primary type issue, which was using `...` instead of `|| {}` - Remove unnecessary `_onClick` cast - Optional switch to optional chaining instead of && check

Error: - This condition will always return true since this function is always defined. Did you mean to call it instead? Fix: - [EuiPinnableListGroup] Remove unnecessary check, pinnable serves that purpose - [EuiOverlayMask] Remove unnecessary check, there's already an early return at the beginning of the block - [extra] DRY out portalTarget var - it's being used interchangably with overlayMaskNode.current

Errors: - Object is of type 'unknown'. - Type 'unknown' is not assignable to type 'EuiMarkdownParseError | null' see https://stackoverflow.com/questions/68240884/error-object-inside-catch-is-of-type-unkown We could optionally disable `useUnknownInCatchVariables`, but I think it also does make sense to check for a valid Error instance if we're going to dive into it.

package.json

cee-chen · 2022-02-02T21:16:57Z

src/components/search_bar/search_bar.tsx

+      const error: Error =
+        e instanceof Error
+          ? { name: e.name, message: e.message }
+          : { name: 'Unexpected error', message: String(e) };


Obligatory https://stackoverflow.com/a/68257472/4294462: In TS 4.4+ caught errors default to unknown instead of any. We could disable this via useUnknownInCatchVariables in tsconfig.json, but honestly I think it's a legit change and it's worth checking for a valid Error instance since something like throw 'foo' is an allowed error. LMK if you have other thoughts however!

Maybe check for 'message' in e - possibly with a type guard to catch some non-error-but-still-compatible objects?

hm, I'd be more tempted to JSON.stringify() a custom error object with a message key - that way the user can always get the full error no matter what. This seems like it might be worth DRYing out a helper that checks for instanceof Error and tries to return a valid Error type if not. WDYT?

Probably too engineery at that point, and leaving as-is makes more sense? 🙂

Fair enough! We can keep it in our back pockets for later if we add more try/catches

kibanamachine · 2022-02-02T21:51:37Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5591/

- it appears to have changed as of the recent upgrade, this keeps all props that output `ReactElement` the same as before

- Was previously working, with, e.g. `number | SomeStringEnum`, now needs a config to restore functionality

kibanamachine · 2022-02-03T18:41:43Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5591/

- it's causing too many other unintended side effects (like turning `|`s into `,`s. There's probably no working around `number | SomeStringEnum` without stating the enum, it'll have to be a props doc changing going forward

kibanamachine · 2022-02-03T20:06:01Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5591/

- enum no longer appears to be extracted by react-docgen

kibanamachine · 2022-02-03T21:05:48Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5591/

- appears to have also changed/shortened since the upgrade

kibanamachine · 2022-02-03T21:54:14Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5591/

It appears as ``` string | number | boolean | {} | ReactElement | ReactNodeArray | ReactPortal ``` in some places, and ``` string | number | boolean | {} | ReactElement | ReactNodeArray | ReactPortal | ({} & string) | (ReactElement<...> & string) | (ReactNodeArray & string) | (ReactPortal & string) ``` in others - this regex should capture both scenarios

- string enums within objects no longer appear to be expanded

kibanamachine · 2022-02-04T03:36:48Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5591/

- EuiButtonDisplay was bogarting the docgenInfo for the file, apparently - moving its declaration below EuiButton and giving EuiButton its a displayName seems to be required to get its docgenInfo working

cee-chen · 2022-02-04T04:39:54Z

OK - I've checked the auto generated props table for all pages/components in our docs 💦 TL;DR: Not all props types changed, but the ones that did were generally for the better/were more concise. Here's a list of bigger changes:

In general the upgrade seems to have caused less extraction/expansion for string enums in unions or objects. In some cases this is a significant improvement, for example EuiDataGrid:

Before:

((props: EuiDataGridCellValueElementProps) => ReactElement
<any, string | 
((props: any) => ReactElement
<any, string | ... | 
(new (props: any) => Component
<any, any, any>)>) | 
(new (props: any) => Component
<...>)>) | 
(new (props: EuiDataGridCellValueElementProps) => Component
<...>) | ((props: EuiDataGridCellValueEleme...

After:

JSXElementConstructor<EuiDataGridCellValueElementProps> | 
((props: EuiDataGridCellValueElementProps) => ReactNode)

And another example:

Before:

string | number | boolean | {} | ReactElement<any, string | 
((props: any) => ReactElement
<any, string | ... | 
(new (props: any) => Component
<any, any, any>)> | null) | 
(new (props: any) => Component
<...>)> | ... 4 more ... | undefined

After:

ReactNode | EuiDataGridToolBarAdditionalControlsOptions

However, in other cases the extra brevity unfortunately leads to less clear docs, e.g. in EuiBasicTable:

Before:
{ field: keyof T; direction: "asc" | "desc"; }

After:
{ field: keyof T; direction: Direction; }

or in EuiFlyout:

Before:
number | "xs" | "s" | "m" | "l" | "xl"

After:
number | EuiBreakpointSize

Interestingly enough, in the case of numeric enums it appears the props docs now do a better job of expanding them (see EuiLoadingContent:

Before:
LineRange

After:
1, 2, 3, 4, 8, 5, 6, 7, 9, 10

It also appears certain Foo<> types are no longer being expanded. For example, some event callbacks:

Before:
(event: MouseEvent<HTMLButtonElement, MouseEvent>) => void

After:
MouseEventHandler<HTMLButtonElement>

However, in other cases like Ref<> this is a readability improvement:

Before:
((instance: HTMLButtonElement) => void)
 | RefObject<HTMLButtonElement> | 
((instance: HTMLAnchorElement) => void)
 | RefObject<...>

After:
Ref<HTMLButtonElement> | Ref<HTMLAnchorElement>

Interestingly enough any CSSProperties in our props became more verbose with an extra *Property<> wrapper. Take this example for CSSProperties['height']:

Before
string | number

After
HeightProperty<string | number>

Omit<> types now prints as-is instead of attempting to Pick<> the non-omitted keys. For example in EuiKeyPadMenu:

Before:
Partial<Pick<EuiToolTipProps, "id" | "children" | "position" | "className" | "onMouseOut" | "display" | "anchorClassName">>

After:
Partial<Omit<EuiToolTipProps, "title" | "content" | "delay">>

Another change I noticed with the upgrade was that certain type references with generics are now correctly expanded, for example EuiSideNav:

Before:
EuiSideNavProps<T>["className"]
EuiSideNavProps<T>["renderItem"]

After:
string
RenderItem<T>

The props table now correctly handles React.* types without them needing to be destructured. Examples can be found in EuiTreeView:

Before:
any

After:
React.ReactElement
React.ReactNode
React.Ref<>
React.MouseEvent

Also worth mentioning, some default values now appear to be properly rendering where they weren't before (e.g. in EuiMarkdownEditor and EuiProvider) which is great and a noted bugfix in react-docgen-typescript's release notes.

kibanamachine · 2022-02-04T05:05:12Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5591/

cee-chen · 2022-02-04T05:12:49Z

src/components/button/button.tsx

+    />
+  );
+};
+EuiButton.displayName = 'EuiButton';


The rundown on this "fix" for EuiButton's props tables - without dede73b, EuiButton's props table errors with docgenInfo is undefined:

This was the only component with this issue that I saw while I was checking every docs page. EuiButtonEmpty, EuiButtonIcon, and EuiButtonGroup did not have this issue. EuiSkipLink had almost the exact same typing (ExclusiveUnion for anchor or button refs) and did not have this issue - a heavy hint that the problem did not lie with the actual types of EuiButton.

Eventually, thanks to Chandler's SUPER helpful tips earlier on how to debug scripts/babel/react-docgen-typescript, I was able to figure out that docgenInfo was, for some reason, only being generated for the internal EuiButtonDisplay component also declared within the same file. So I experimented with temporarily moving it out of the file, and boom. EuiButton's props table started working again.

I could have moved EuiButtonDisplay out to its own button_display file (and still can, if people prefer) but opted for the fix with the least diffs. For whatever reason, if I moved EuiButtonDisplay down on the page and also gave EuiButton a displayName, that was enough for react-docgen-typescript to look at EuiButton first and ignore EuiButtonDisplay. 🙈

EDIT: Aha, after digging more through react-docgen-typescripts, it looks like this is a known issue on their end that was introduced in 2.1.0: styleguidist/react-docgen-typescript#395

@chandlerprall any thoughts on whether we should document this limitation for now until react-docgen-typescript has a fix?

I updated our component development wiki: 97a5cd8

With all that, I think this PR is ready to go if we're OK with all of the above type changes, and if someone else could take a quick second QA pass through this PR's staging server to check that there are no other obviously broken prop tables.

Ouch, that's a rough bug. Thanks for updating the wiki page, should be enough to help track this and point people to if/when needed.

- Add warning about displayName bug found in elastic@dede73b#r799165839 - useful links to our custom react-docgen config/filters + react's docs - lint misc typos/grammar

kibanamachine · 2022-02-04T06:07:16Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5591/

thompsongl

This all looks great! Quickly looked through all the props tables, mostly to ensure no errors.

Not all props types changed, but the ones that did were generally for the better/were more concise

Agreed!

However, in other cases the extra brevity unfortunately leads to less clear docs

I think we're ok to make per-case judgment calls on which enum style to use 👍

Also worth mentioning, some default values now appear to be properly rendering where they weren't before

This is probably my favorite improvement

any thoughts on whether we should document this limitation for now until react-docgen-typescript has a fix?

I'm good with the documentation addition. It's rare that we're declaring multiple components in a file, let alone multiple with displayName declarations.

chandlerprall

Changes LGTM, didn't try to break anything

cee-chen · 2022-02-07T16:10:19Z

Woohoo! Thanks y'all. Merging.

cee-chen added 7 commits February 2, 2022 13:02

Upgrade typescript to 4.5.3

be0ee89

- to match Kibana

Fix typescript err TS2339

02026e6

Property 'MSInputMethodContext' does not exist on type 'Window & typeof globalThis'

Fix typescript error TS2783

bcb4420

Error: 'onClick' is specified more than once, so this usage will be overwritten.

Update typescript related dependencies

a607fdf

cee-chen added the tech debt label Feb 2, 2022

cee-chen requested review from chandlerprall and thompsongl February 2, 2022 21:09

cee-chen mentioned this pull request Feb 2, 2022

Upgrade to React 17 #5584

Merged

8 tasks

cee-chen commented Feb 2, 2022

View reviewed changes

cee-chen added the breaking change label Feb 2, 2022

cee-chen added 4 commits February 3, 2022 09:19

Merge branch 'main' into typescript

d12815c

Changelog

e217741

Update reactElementTypeExpanded replacement

1d29ce0

- it appears to have changed as of the recent upgrade, this keeps all props that output `ReactElement` the same as before

Expand string enums that are used within unions

58826b6

- Was previously working, with, e.g. `number | SomeStringEnum`, now needs a config to restore functionality

Revert shouldExtractValuesFromUnion

8a6352e

- it's causing too many other unintended side effects (like turning `|`s into `,`s. There's probably no working around `number | SomeStringEnum` without stating the enum, it'll have to be a props doc changing going forward

Table auto props docs - spell out direction explicitly

3779869

- enum no longer appears to be extracted by react-docgen

Fix other reactNodeTypeExpanded replace

3684d2c

- appears to have also changed/shortened since the upgrade

cee-chen added 2 commits February 3, 2022 18:56

Portal auto props docs - spell out position explicitly

1a6aa52

- string enums within objects no longer appear to be expanded

Fix broken EuiButton props table

dede73b

- EuiButtonDisplay was bogarting the docgenInfo for the file, apparently - moving its declaration below EuiButton and giving EuiButton its a displayName seems to be required to get its docgenInfo working

cee-chen commented Feb 4, 2022

View reviewed changes

Improve react-docgen-typescript wiki docs

97a5cd8

- Add warning about displayName bug found in elastic@dede73b#r799165839 - useful links to our custom react-docgen config/filters + react's docs - lint misc typos/grammar

cee-chen force-pushed the typescript branch from 85ea67d to 97a5cd8 Compare February 4, 2022 05:30

thompsongl approved these changes Feb 4, 2022

View reviewed changes

chandlerprall approved these changes Feb 4, 2022

View reviewed changes

cee-chen merged commit aa4cfd7 into elastic:main Feb 7, 2022

cee-chen deleted the typescript branch February 7, 2022 16:10

breehall mentioned this pull request Feb 8, 2022

Upgrade EUI to v48.1.1 elastic/kibana#125023

Merged

cee-chen mentioned this pull request Feb 14, 2022

[EuiTablePagination] Adding a Show all option; Plus Pagination Guidelines #5547

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Typescript to 4.5.3 to match Kibana #5591

Upgrade Typescript to 4.5.3 to match Kibana #5591

cee-chen commented Feb 2, 2022 •

edited

Loading

cee-chen Feb 2, 2022 •

edited

Loading

chandlerprall Feb 2, 2022

cee-chen Feb 2, 2022 •

edited

Loading

chandlerprall Feb 3, 2022

cee-chen Feb 3, 2022

kibanamachine commented Feb 2, 2022

kibanamachine commented Feb 3, 2022

kibanamachine commented Feb 3, 2022

kibanamachine commented Feb 3, 2022

kibanamachine commented Feb 3, 2022

kibanamachine commented Feb 4, 2022

cee-chen commented Feb 4, 2022 •

edited

Loading

kibanamachine commented Feb 4, 2022

cee-chen Feb 4, 2022

cee-chen Feb 4, 2022 •

edited

Loading

chandlerprall Feb 4, 2022

kibanamachine commented Feb 4, 2022

thompsongl left a comment •

edited

Loading

chandlerprall left a comment

cee-chen commented Feb 7, 2022

Upgrade Typescript to 4.5.3 to match Kibana #5591

Upgrade Typescript to 4.5.3 to match Kibana #5591

Conversation

cee-chen commented Feb 2, 2022 • edited Loading

Summary

Checklist

cee-chen Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

chandlerprall Feb 2, 2022

Choose a reason for hiding this comment

cee-chen Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

chandlerprall Feb 3, 2022

Choose a reason for hiding this comment

cee-chen Feb 3, 2022

Choose a reason for hiding this comment

kibanamachine commented Feb 2, 2022

kibanamachine commented Feb 3, 2022

kibanamachine commented Feb 3, 2022

kibanamachine commented Feb 3, 2022

kibanamachine commented Feb 3, 2022

kibanamachine commented Feb 4, 2022

cee-chen commented Feb 4, 2022 • edited Loading

kibanamachine commented Feb 4, 2022

cee-chen Feb 4, 2022

Choose a reason for hiding this comment

cee-chen Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

chandlerprall Feb 4, 2022

Choose a reason for hiding this comment

kibanamachine commented Feb 4, 2022

thompsongl left a comment • edited Loading

Choose a reason for hiding this comment

chandlerprall left a comment

Choose a reason for hiding this comment

cee-chen commented Feb 7, 2022

cee-chen commented Feb 2, 2022 •

edited

Loading

cee-chen Feb 2, 2022 •

edited

Loading

cee-chen Feb 2, 2022 •

edited

Loading

cee-chen commented Feb 4, 2022 •

edited

Loading

cee-chen Feb 4, 2022 •

edited

Loading

thompsongl left a comment •

edited

Loading