Skip to content

fix(ui): Typescript 4 misc fixes #20703

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

Jump to bottom
Merged
merged 7 commits into from
Sep 11, 2020
Merged

fix(ui): Typescript 4 misc fixes #20703

merged 7 commits into from
Sep 11, 2020

Conversation

scttcper
Copy link
Member

@scttcper scttcper commented Sep 10, 2020 •
edited
Loading

This is pretty much all of them except the HoF issues in #20701

The most common issue was with deleting a property that is not optional.

@vercel
Copy link

vercel bot commented Sep 10, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

@scttcper scttcper mentioned this pull request Sep 10, 2020
Closed
scttcper
scttcper commented Sep 10, 2020
View reviewed changes
src/sentry/static/sentry/app/components/button.tsx
@@ -358,7 +358,7 @@ const getIconMargin = ({size, hasChildren}: IconProps) => {
return size && size.endsWith('small') ? '6px' : '8px';
};

const Icon = styled('span')<IconProps>`
const Icon = styled('span')<IconProps & Omit<StyledButtonProps, 'theme'>>`
Copy link
Member Author

Choose a reason for hiding this comment

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

needed all the types for getFontSize and getIconMargin

@github-actions
Copy link
Contributor

github-actions bot commented Sep 10, 2020 •
edited
Loading

size-limit report

Path Size
public/app.js 233.68 KB (0%)
public/vendor.js 444.55 KB (0%)

scttcper
scttcper commented Sep 10, 2020
View reviewed changes
src/sentry/static/sentry/app/components/events/groupingInfo/utils.tsx
@@ -15,7 +15,7 @@ export function hasNonContributingComponent(component: EventGroupComponent | und
}

export function shouldInlineComponentValue(component: EventGroupComponent) {
return component.values.every(value => !isObject(value));
return (component.values as EventGroupComponent[]).every(value => !isObject(value));
Copy link
Member Author

@scttcper scttcper Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

wasn't sure on this, but it didn't like calling .every on type EventGroupComponent[] | string[] possibly because EventGroupComponent was self referencing

Copy link
Member Author

@scttcper scttcper Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

typescript playground of this weird one

scttcper
scttcper commented Sep 10, 2020
View reviewed changes
src/sentry/static/sentry/app/components/events/interfaces/spans/utils.tsx
@@ -307,6 +307,8 @@ export const setBodyUserSelect = (nextValues: UserSelectValues): UserSelectValue
// MozUserSelect is not typed in TS
// @ts-ignore
MozUserSelect: document.body.style.MozUserSelect,
// msUserSelect is not typed in TS
// @ts-ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

msUserSelect is now also missing in ts 4

Copy link
Member

Choose a reason for hiding this comment

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

Any idea why? Should we be dropping support too?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://caniuse.com/user-select-none

Seems like we could

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this.

I'm aware of this being a missing type in TypeScript. This was causing an issue now because of updated type definitions for user select on v4.

scttcper
scttcper commented Sep 10, 2020
View reviewed changes
src/sentry/static/sentry/app/utils/discover/eventView.tsx
@@ -918,7 +918,7 @@ class EventView {
const environment = this.environment as string[];

// generate event query
const eventQuery: EventQuery & LocationQuery = Object.assign(
const eventQuery = Object.assign(
Copy link
Member Author

@scttcper scttcper Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This errors in ts4

i think this change is more correct, eventQuery isn't EventQuery & LocationQuery, but instead we're casting the result from object.assign to EventQuery & LocationQuery

scttcper
scttcper commented Sep 10, 2020
View reviewed changes
src/sentry/static/sentry/app/views/eventsV2/queryList.tsx
@@ -235,7 +235,7 @@ class QueryList extends React.Component<Props> {
onCursor={(cursor: string, path: string, query: Query, direction: number) => {
const offset = Number(cursor.split(':')[1]);

const newQuery = {...query, cursor};
const newQuery: Query & {cursor?: string} = {...query, cursor};
Copy link
Member Author

Choose a reason for hiding this comment

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

allow deletion via optional

billyvg
billyvg reviewed Sep 10, 2020
View reviewed changes
src/sentry/static/sentry/app/views/eventsV2/eventDetails/content.tsx
@@ -127,7 +127,7 @@ class EventDetailsContent extends AsyncComponent<Props, State> {
}
const eventReference = {...event};
if (eventReference.id) {
delete eventReference.id;
delete (eventReference as any).id;
Copy link
Member

Choose a reason for hiding this comment

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

What about refactoring the spread to something like

{id:_, ...eventReference} = event

Copy link
Member Author

Choose a reason for hiding this comment

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

eventReference is passed to functions that then expect Event type. Without the id it errors as well. Could change the type of Event but that also feels like a bigger change.

src/sentry/static/sentry/app/components/events/interfaces/spans/utils.tsx
@@ -307,6 +307,8 @@ export const setBodyUserSelect = (nextValues: UserSelectValues): UserSelectValue
// MozUserSelect is not typed in TS
// @ts-ignore
MozUserSelect: document.body.style.MozUserSelect,
// msUserSelect is not typed in TS
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Any idea why? Should we be dropping support too?

@vercel vercel bot had a problem deploying to Preview – storybook September 10, 2020 15:22 Failure
@vercel vercel bot had a problem deploying to Preview – storybook September 10, 2020 16:17 Failure
@vercel vercel bot had a problem deploying to Preview – storybook September 10, 2020 16:54 Failure
@scttcper scttcper marked this pull request as ready for review September 10, 2020 17:29
@scttcper scttcper requested a review from a team as a code owner September 10, 2020 17:29
@scttcper scttcper requested a review from a team September 10, 2020 17:29
@vercel vercel bot had a problem deploying to Preview – storybook September 10, 2020 17:37 Failure
markstory
markstory approved these changes Sep 11, 2020
View reviewed changes
Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

@scttcper
Merge branch 'master' into scttcper/typescript-4-fixes
aa4a8c6 
# Conflicts:
#	src/sentry/static/sentry/app/data/forms/projectGeneralSettings.tsx
@scttcper scttcper merged commit 4fa8f63 into master Sep 11, 2020
@scttcper scttcper deleted the scttcper/typescript-4-fixes branch September 11, 2020 17:18
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@billyvg billyvg billyvg left review comments

@dashed dashed dashed left review comments

@markstory markstory markstory approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@scttcper @markstory @billyvg @dashed