chore(ts): Convert src/sentry/static/sentry/app/utils.jsx to typescript by dashed · Pull Request #14199 · getsentry/sentry

dashed · 2019-07-30T03:45:02Z

TODO

review descopeFeatureName()

Deferred to another followup PR

lynnagara · 2019-07-30T03:50:02Z

src/sentry/static/sentry/app/utils.tsx

-function objectMatchesSubset(obj, other, deep) {
-  let k;
+function objectMatchesSubset(obj?: object, other?: object, deep?: boolean): boolean {
+  let k: string | number;


Why number, shouldn't it always be string?

@lynnagara I thought so too. I used vscode to infer the type for me:

Is this because array is a valid type of object (keys are just numbers instead of strings?). In our usage of this function I think it should only be an object with key as string.

I made k a string.

lynnagara · 2019-07-30T03:53:56Z

src/sentry/static/sentry/app/utils.tsx

 }

-export function intcomma(x) {
+export function intcomma(x: object): string {


x should be number?

lynnagara · 2019-07-30T03:55:17Z

src/sentry/static/sentry/app/utils.tsx

-  }
-  return result;
+export function objectToArray<T>(
+  obj: {[s: string]: T} | ArrayLike<T>


When is it not a regular object?

I'm just going to use export const objectToArray = Object.entries;

lynnagara · 2019-07-30T04:00:58Z

src/sentry/static/sentry/app/utils.tsx

 }

-function projectDisplayCompare(a, b) {
+interface Project {


Can we use https://github.com/getsentry/sentry/blob/master/src/sentry/static/sentry/app/types/index.tsx#L8?

lynnagara · 2019-07-30T04:02:57Z

src/sentry/static/sentry/app/utils.tsx

-  return typeof feature.match !== 'function'
-    ? feature
-    : feature.match(/(?:^(?:projects|organizations):)?(.*)/).pop();
+export function descopeFeatureName(feature: string): string {


This (feature) seems like it's not always a string

where is it not a string?

If it's always a string then line 222 - 224 doesn't make sense

Ah hm, I forget why I wrote it like that 😬

I've adjusted the types for this function

lynnagara · 2019-07-30T04:05:24Z

src/sentry/static/sentry/app/utils.tsx

  }

-  return Object.freeze(object);
+  return Object.freeze<T>(object);


Is there value in this type annotation? We could probably drop this

dashed · 2019-07-30T04:10:52Z

src/sentry/static/sentry/app/utils.tsx

    const match = repo.match(re);
    const parsedRepo = match ? match[1] : repo;
-    return parsedRepo;
+    return parsedRepo as any;


So this needs microsoft/TypeScript#22348

lynnagara · 2019-07-30T05:15:55Z

src/sentry/static/sentry/app/types/index.tsx

  slug: string;
  isMember: boolean;
+
+  isBookmarked?: boolean;


Might not need to be optional? AFAIK we always get an isBookmarked value for a project

markstory · 2019-07-30T13:34:37Z

src/sentry/static/sentry/app/utils.tsx

-  }
-  return result;
-}
+export const objectToArray = Object.entries;


Could we refactor this method away? I don't see there being much value in having an alias to stdlib.

evanpurkhiser · 2019-07-30T18:23:31Z

src/sentry/static/sentry/app/utils.tsx

-  }
-  return result;
-}
+export const objectToArray = Object.entries;


I wonder if this will have implications, since right now this would iterate keys like constructor. Not sure if Object.entries will do that as well.

I don't think we ever want to iterate keys like constructor, Object.entries only iterates its own properties.

Good points. I'll revisit this to see if the refactor is generally small enough to fit into this PR.

I don't want to accidentally introduce a bug by "correcting" this utility function.

Let's not tackle it here, IMO we should fix it though, I recall a case of a project named "constructor" recently that did have some problems.

evanpurkhiser · 2019-07-30T18:32:52Z

src/sentry/static/sentry/app/utils.tsx

+    return results.pop()!;
+  }
+
+  return feature;


Personal preference, but I think the original shorter version was easier to read (But I'm also biased, because I wrote it)

Even this last bit I think could just be return results ? results.pop() : feature

@evanpurkhiser oh interesting. I'll revisit this.

Typescript indicated that the match() can return null, which is why I rewrote it like this.

Ahhh, yeah makes sense.

dashed requested a review from a team July 30, 2019 03:45

dashed self-assigned this Jul 30, 2019

lynnagara reviewed Jul 30, 2019

View reviewed changes

dashed commented Jul 30, 2019

View reviewed changes

lynnagara reviewed Jul 30, 2019

View reviewed changes

markstory reviewed Jul 30, 2019

View reviewed changes

evanpurkhiser reviewed Jul 30, 2019

View reviewed changes

dashed added the WIP label Jul 31, 2019

dashed force-pushed the typescript/app-utils branch 2 times, most recently from d97d3f3 to 544f071 Compare August 8, 2019 17:23

dashed requested review from evanpurkhiser, lynnagara and markstory August 8, 2019 18:07

dashed removed the WIP label Aug 8, 2019

dashed force-pushed the typescript/app-utils branch from 157984f to 496fb5c Compare August 8, 2019 18:58

dashed mentioned this pull request Aug 9, 2019

chore(ts): Initial typescript conversion in preparation of discover v2 #14345

Merged

8 tasks

dashed force-pushed the typescript/app-utils branch 2 times, most recently from 3b25e34 to 0f2f85a Compare August 12, 2019 19:53

lynnagara approved these changes Aug 13, 2019

View reviewed changes

dashed force-pushed the typescript/app-utils branch from 0f2f85a to 76c29ca Compare August 13, 2019 14:46

chore(ts): Convert src/sentry/static/sentry/app/utils.jsx to typescript

92b3a4f

dashed force-pushed the typescript/app-utils branch from 76c29ca to 92b3a4f Compare August 13, 2019 19:41

dashed merged commit f21520b into master Aug 13, 2019

dashed deleted the typescript/app-utils branch August 13, 2019 20:56

dashed mentioned this pull request Aug 13, 2019

chore(js): Replace all instances of objectToArray with Object.entries #14380

Merged

1 task

github-actions bot locked and limited conversation to collaborators Dec 20, 2020

Uh oh!

Conversation

dashed commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Deferred to another followup PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lynnagara Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lynnagara Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dashed commented Jul 30, 2019 •

edited

Loading

lynnagara Jul 30, 2019 •

edited

Loading

lynnagara Aug 1, 2019 •

edited

Loading