Skip to content
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.

Sign up for GitHub

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

Bump typescript to 4.6.4 #879

Merged
merged 23 commits into from
Sep 28, 2023
Merged

Bump typescript to 4.6.4 #879

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
eda846e
typescript bump: 4.1.6
gulderov Jul 18, 2023
6da59ae
typescript bump: 4.2.4
gulderov Jul 18, 2023
281aff0
typescript bump: 4.3.5
gulderov Jul 18, 2023
94b257d
typescript bump: 4.4.4
gulderov Jul 18, 2023
32383d6
typescript bump: 4.5.5
gulderov Jul 18, 2023
a19f309
typescript bump: 4.6.4
gulderov Jul 18, 2023
f4eea57
CHANGELOG.md updated
gulderov Jul 18, 2023
e8e7915
bump typescript-eslint/eslint-plugin: 5.62.0
gulderov Jul 18, 2023
c445087
CHANGELOG.md update
gulderov Jul 18, 2023
cdc3344
move typescript update to regular section
gulderov Jul 18, 2023
84d5c7e
changelog message improvement
gulderov Jul 18, 2023
8fba865
fix typings
gulderov Jul 18, 2023
df82abe
cleanup useUnknownInCatchVariables
gulderov Jul 18, 2023
1885553
logical or => nullish coalescing
gulderov Jul 19, 2023
ed34ef5
wrap with String
gulderov Jul 20, 2023
4115c19
Merge branch 'main' into ts-bump
BSFishy Jul 20, 2023
c707631
Merge branch 'main' into ts-bump
joshuarrrr Sep 13, 2023
5dad068
Merge branch 'main' into ts-bump
gulderov Sep 20, 2023
1450d66
Update CHANGELOG.md
AMoo-Miki Sep 27, 2023
dbfb6b4
Add back `onPinClick` check in `OuiPinnableListGroup`
AMoo-Miki Sep 27, 2023
23dde58
Enhance error message extraction in `validateFieldValue`
AMoo-Miki Sep 27, 2023
2c4d894
Remove e.code from default_syntax.ts
AMoo-Miki Sep 27, 2023
4be4cbb
Lint default_syntax.ts
AMoo-Miki Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@

### 🛠 Maintenance

- Bump TypeScript to v4.6.4 ([#879](https://github.com/opensearch-project/oui/pull/879))
AMoo-Miki marked this conversation as resolved.
Show resolved Hide resolved
- [CVE-2023-26136] Add resolution for tough-cookie to ^4.1.3 ([#889](https://github.com/opensearch-project/oui/pull/889))
- [CVE-2023-26115] Bump word-wrap from 1.2.3 to 1.2.4 ([#891](https://github.com/opensearch-project/oui/pull/891))
- Bump Node version to 18.16.0 ([#900](https://github.com/opensearch-project/oui/pull/900))
Expand Down
28 changes: 14 additions & 14 deletions i18ntokens.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2543,14 +2543,14 @@
"highlighting": "string",
"loc": {
"start": {
"line": 125,
"line": 124,
"column": 4,
"index": 3690
"index": 3665
},
"end": {
"line": 125,
"line": 124,
"column": 75,
"index": 3761
"index": 3736
}
},
"filepath": "src/components/header/header_links/header_links.tsx"
Expand All @@ -2561,14 +2561,14 @@
"highlighting": "string",
"loc": {
"start": {
"line": 138,
"line": 137,
"column": 4,
"index": 4083
"index": 4058
},
"end": {
"line": 138,
"line": 137,
"column": 69,
"index": 4148
"index": 4123
}
},
"filepath": "src/components/header/header_links/header_links.tsx"
Expand Down Expand Up @@ -2653,12 +2653,12 @@
"start": {
"line": 143,
"column": 4,
"index": 4714
"index": 4700
},
"end": {
"line": 148,
"column": 44,
"index": 4899
"index": 4885
}
},
"filepath": "src/components/list_group/pinnable_list_group/pinnable_list_group.tsx"
Expand All @@ -2671,12 +2671,12 @@
"start": {
"line": 143,
"column": 4,
"index": 4714
"index": 4700
},
"end": {
"line": 148,
"column": 44,
"index": 4899
"index": 4885
}
},
"filepath": "src/components/list_group/pinnable_list_group/pinnable_list_group.tsx"
Expand Down Expand Up @@ -3535,12 +3535,12 @@
"start": {
"line": 267,
"column": 12,
"index": 7947
"index": 7990
},
"end": {
"line": 270,
"column": 14,
"index": 8069
"index": 8112
}
},
"filepath": "src/components/selectable/selectable_templates/selectable_template_sitewide.tsx"
Expand Down
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@
"@types/react-router-dom": "^5.3.3",
"@types/url-parse": "^1.4.8",
"@types/uuid": "^9.0.1",
"@typescript-eslint/eslint-plugin": "^4.8.1",
"@typescript-eslint/parser": "^4.8.1",
"@typescript-eslint/eslint-plugin": "^5.62.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The @typescript-eslint/eslint-plugin: "^6.0.0" requires at least node v16, but .nvmrc contains 14.20.0

"@typescript-eslint/parser": "^5.62.0",
"autoprefixer": "^9.8.6",
"babel-core": "7.0.0-bridge.0",
"babel-eslint": "^10.1.0",
Expand Down Expand Up @@ -245,7 +245,7 @@
"start-server-and-test": "^2.0.0",
"style-loader": "^1.2.1",
"terser-webpack-plugin": "^4.1.0",
"typescript": "4.0.5",
"typescript": "4.6.4",
"url-loader": "^4.1.0",
"webpack": "npm:@amoo-miki/[email protected]",
"webpack-cli": "^3.3.12",
Expand Down
2 changes: 1 addition & 1 deletion src-docs/src/components/guide_section/guide_section.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ export const GuideSection: FunctionComponent<GuideSection> = ({
const isPlaygroundUnsupported =
typeof window !== 'undefined' &&
typeof document !== 'undefined' &&
!!window.MSInputMethodContext &&
!!(window as any).MSInputMethodContext &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love adding new code with any. Could this be cast as object or unknown instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, we could consider completely removing this. Please look at the follow up issue #886

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, we can add the following code to index.d.ts

interface Window {
  MSInputMethodContext?: any; // is IE11
}

interface Document {
  documentMode?: any; // is IE11
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point about fixing it by adding to global types. The usage of any is common to highlight areas of uncertainty or complexity in typescript. It is usually recommended to avoid it if possible

In this case it should be safe to mark some pain point by as any. It could be more scary to cleanup index.d.ts in future, than cleanup local usage of any. My preference is to keep theindex.d.ts nice and clean.

On other hand I cant see better cast here and we have issue #886 for refactoring

// @ts-ignore doesn't exist?
!!document.documentMode;

Expand Down
6 changes: 1 addition & 5 deletions src/components/datagrid/data_grid_inmemory_renderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,7 @@ export interface OuiDataGridInMemoryRendererProps {
function noop() {}

function getElementText(element: HTMLElement) {
return 'innerText' in element
? element.innerText
: // (this line left here to satisfy Prettier since a ts-ignore is used on the next line)
// @ts-ignore TypeScript thinks element.innerText always exists, however it doesn't in jest/jsdom environment
element.textContent || undefined;
Comment on lines -59 to -63
Copy link
Contributor

Choose a reason for hiding this comment

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

Love that we're able to get rid of this @ts-ignore

return 'innerText' in element ? element.innerText : element.textContent || '';
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we're changing the return type to string from string | undefined - is this intentional, and does it have any effect in caller contexts?

Copy link
Collaborator

@AMoo-Miki AMoo-Miki Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

While i think it will impact the caller contexts, i believe the previous behavior of returning undefined was wrong.

}

export const OuiDataGridInMemoryRenderer: FunctionComponent<OuiDataGridInMemoryRendererProps> = ({
Expand Down
7 changes: 3 additions & 4 deletions src/components/header/header_links/header_links.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,16 +95,15 @@ export const OuiHeaderLinks: FunctionComponent<OuiHeaderLinksProps> = ({
popoverProps,
...rest
}) => {
const { onClick: _onClick, iconType = 'apps', ...popoverButtonRest } = {
...popoverButtonProps,
};
const { onClick, iconType = 'apps', ...popoverButtonRest } =
popoverButtonProps ?? {};

const [mobileMenuIsOpen, setMobileMenuIsOpen] = useState(false);

const onMenuButtonClick: MouseEventHandler<
HTMLButtonElement & HTMLAnchorElement
> = (e) => {
_onClick && _onClick(e);
onClick?.(e);
setMobileMenuIsOpen(!mobileMenuIsOpen);
};

Expand Down
2 changes: 1 addition & 1 deletion src/components/icon/icon.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const prettyHtml = cheerio.load('');
function testIcon(props: PropsOf<OuiIcon>) {
return () => {
expect.assertions(1);
return new Promise((resolve) => {
return new Promise<void>((resolve) => {
const onIconLoad = () => {
component.update();
expect(prettyHtml(component.html())).toMatchSnapshot();
Expand Down
2 changes: 1 addition & 1 deletion src/components/list_group/pinnable_list_group/pinnable_list_group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export const OuiPinnableListGroup: FunctionComponent<OuiPinnableListGroupProps>
);

// Add the pinning action unless the item has it's own extra action
if (onPinClick && !itemProps.extraAction && pinnable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is onPinClick removed here? From a brief look at the code, it seems like it should be null checked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The onPinClick is defined as required prop for OuiPinnableListGroup component. Typescript will give error, if onPinClick is not provided. So no extra validation needed. I hope we can benefit from type checking here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Compile-time warning cannot take the place of proper guarding. There are many scenarios that TS will not be able check during compiling.

That said, onPinClick check shouldn't be up there but should be added down.

if (onPinClick) itemProps.extraAction.onClick = () => onPinClick(item);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I committed the above suggestion.

if (pinnable && !itemProps.extraAction) {
// Different displays for pinned vs unpinned
if (pinned) {
itemProps.extraAction = {
Expand Down
2 changes: 1 addition & 1 deletion src/components/markdown_editor/markdown_editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export const OuiMarkdownEditor = forwardRef<
const parsed = parser.processSync(value);
return [parsed, null];
} catch (e) {
return [null, e];
return [null, e as OuiMarkdownParseError];
}
}, [parser, value]);

Expand Down
4 changes: 3 additions & 1 deletion src/components/search_bar/query/default_syntax.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,8 +353,10 @@ const validateFieldValue = (
try {
schemaField.validate(value);
} catch (e) {
const message = e instanceof Error ? e.message : String(e);

error(
`Invalid value \`${expression}\` set for field \`${field}\` - ${e.message}`,
`Invalid value \`${expression}\` set for field \`${field}\` - ${message}`,
location
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's strengthen this a bit:

     const message = (e instanceof Error ? e.message || e.code : String(e)).trim();

      error(
        `Invalid value \`${expression}\` set for field \`${field}\`${message ? ` - ${message}` : '' }`,
        location
      );

Copy link
Collaborator

@AMoo-Miki AMoo-Miki Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I committed a nicer version of the above suggestion.
PS, tsc didn't like that. Will adjust it.

}
Expand Down
6 changes: 5 additions & 1 deletion src/components/search_bar/search_bar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,11 @@ export class OuiSearchBar extends Component<OuiSearchBarProps, State> {
this.notifyControllingParent({ query, queryText, error: null });
this.setState({ query, queryText, error: null });
} catch (e) {
const error: Error = { name: e.name, message: e.message };
const error: Error =
e instanceof Error
? { name: e.name, message: e.message }
: { name: 'Unexpected error', message: String(e) };

this.notifyControllingParent({ query: null, queryText, error });
this.setState({ queryText, error });
}
Expand Down
2 changes: 1 addition & 1 deletion src/components/selectable/selectable_templates/selectable_template_sitewide.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ export const OuiSelectableTemplateSitewide: FunctionComponent<OuiSelectableTempl
searchProps={{
placeholder: searchPlaceholder,
isClearable: true,
...searchProps,
...(searchProps as Omit<typeof searchProps, 'className'>),
onFocus: searchOnFocus,
onBlur: searchOnBlur,
onInput: onSearchInput,
Expand Down
10 changes: 2 additions & 8 deletions src/components/suggest/suggest_item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import React, {
FunctionComponent,
HTMLAttributes,
ButtonHTMLAttributes,
MouseEventHandler,
} from 'react';
import { CommonProps, ExclusiveUnion, keysOf } from '../common';
import classNames from 'classnames';
Expand Down Expand Up @@ -77,12 +76,7 @@ interface OuiSuggestItemPropsBase {
}

type PropsForDiv = Omit<HTMLAttributes<HTMLDivElement>, 'onClick'>;
type PropsForButton = Omit<
ButtonHTMLAttributes<HTMLButtonElement>,
'onClick' | 'type'
> & {
onClick: MouseEventHandler<HTMLButtonElement> | undefined;
};
type PropsForButton = Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'type'>;

export type OuiSuggestItemProps = CommonProps &
OuiSuggestItemPropsBase &
Expand Down Expand Up @@ -193,8 +187,8 @@ export const OuiSuggestItem: FunctionComponent<OuiSuggestItemProps> = ({
if (onClick) {
return (
<button
onClick={onClick}
className={classes}
onClick={onClick}
{...(rest as PropsForButton)}>
{innerContent}
</button>
Expand Down
Loading