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.

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

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

Merged
merged 1 commit into from
Aug 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/sentry/static/sentry/app/types/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ export type Project = {
isMember: boolean;
teams: Team[];
features: string[];

isBookmarked: boolean;
};

export type Team = {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import _ from 'lodash';
import {Project} from 'app/types/index';

function arrayIsEqual(arr, other, deep) {
function arrayIsEqual(arr?: any[], other?: any[], deep?: boolean): boolean {
// if the other array is a falsy value, return
if (!arr && !other) {
return true;
Expand All @@ -18,7 +19,7 @@ function arrayIsEqual(arr, other, deep) {
return arr.every((val, idx) => valueIsEqual(val, other[idx], deep));
}

export function valueIsEqual(value, other, deep) {
export function valueIsEqual(value?: any, other?: any, deep?: boolean): boolean {
if (value === other) {
return true;
} else if (_.isArray(value) || _.isArray(other)) {
Expand All @@ -33,8 +34,8 @@ export function valueIsEqual(value, other, deep) {
return false;
}

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

if (obj === other) {
return true;
Expand All @@ -61,21 +62,14 @@ function objectMatchesSubset(obj, other, deep) {
return true;
}

// XXX(dcramer): the previous mechanism of using _.map here failed
// miserably if a param was named 'length'
export function objectToArray(obj) {
const result = [];
for (const key in obj) {
result.push([key, obj[key]]);
}
return result;
}
// TODO: instances of objectToArray should be refactored
export const objectToArray = Object.entries;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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 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.


export function intcomma(x) {
export function intcomma(x: number): string {
return x.toString().replace(/\B(?=(\d{3})+(?!\d))/g, ',');
}

export function sortArray(arr, score_fn) {
export function sortArray<T>(arr: Array<T>, score_fn: (entry: T) => string): Array<T> {
arr.sort((a, b) => {
const a_score = score_fn(a),
b_score = score_fn(b);
Expand All @@ -94,7 +88,7 @@ export function sortArray(arr, score_fn) {
return arr;
}

export function objectIsEmpty(obj) {
export function objectIsEmpty(obj: object): boolean {
for (const prop in obj) {
if (obj.hasOwnProperty(prop)) {
return false;
Expand All @@ -104,55 +98,55 @@ export function objectIsEmpty(obj) {
return true;
}

export function trim(str) {
export function trim(str: string): string {
return str.replace(/^\s+|\s+$/g, '');
}

/**
* Replaces slug special chars with a space
*/
export function explodeSlug(slug) {
export function explodeSlug(slug: string): string {
return trim(slug.replace(/[-_]+/g, ' '));
}

export function defined(item) {
export function defined(item: any): boolean {
return !_.isUndefined(item) && item !== null;
}

export function nl2br(str) {
export function nl2br(str: string): string {
return str.replace(/(?:\r\n|\r|\n)/g, '<br />');
}

/**
* This function has a critical security impact, make sure to check all usages before changing this function.
* In some parts of our code we rely on that this only really is a string starting with http(s).
*/
export function isUrl(str) {
export function isUrl(str: any): boolean {
return (
!!str &&
_.isString(str) &&
(str.indexOf('http://') === 0 || str.indexOf('https://') === 0)
);
}

export function escape(str) {
export function escape(str: string): string {
return str
.replace(/&/g, '&amp;')
.replace(/</g, '&lt;')
.replace(/>/g, '&gt;');
}

export function percent(value, totalValue, precise) {
export function percent(value: number, totalValue: number): number {
return (value / totalValue) * 100;
}

export function toTitleCase(str) {
export function toTitleCase(str: string): string {
return str.replace(/\w\S*/g, txt => {
return txt.charAt(0).toUpperCase() + txt.substr(1).toLowerCase();
});
}

export function formatBytes(bytes) {
export function formatBytes(bytes: number): string {
const units = ['KB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB'];
const thresh = 1024;
if (bytes < thresh) {
Expand All @@ -167,7 +161,7 @@ export function formatBytes(bytes) {
return bytes.toFixed(1) + ' ' + units[u];
}

export function getShortVersion(version) {
export function getShortVersion(version: string): string {
if (version.length < 12) {
return version;
}
Expand All @@ -184,37 +178,37 @@ export function getShortVersion(version) {
return version;
}

export function parseRepo(repo) {
if (!repo) {
return repo;
} else {
export function parseRepo<T>(repo: T): T {
if (typeof repo === 'string') {
const re = /(?:github\.com|bitbucket\.org)\/([^\/]+\/[^\/]+)/i;
const match = repo.match(re);
const parsedRepo = match ? match[1] : repo;
return parsedRepo;
return parsedRepo as any;
Copy link
Member Author

Choose a reason for hiding this comment

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

}

return repo;
}

/**
* Converts a multi-line textarea input value into an array,
* eliminating empty lines
*/
export function extractMultilineFields(value) {
export function extractMultilineFields(value: string): Array<string> {
return value
.split('\n')
.map(f => trim(f))
.filter(f => f !== '');
}

function projectDisplayCompare(a, b) {
function projectDisplayCompare(a: Project, b: Project): number {
if (a.isBookmarked !== b.isBookmarked) {
return a.isBookmarked ? -1 : 1;
}
return a.slug.localeCompare(b.slug);
}

// Sort a list of projects by bookmarkedness, then by id
export function sortProjects(projects) {
export function sortProjects(projects: Array<Project>): Array<Project> {
return projects.sort(projectDisplayCompare);
}

Expand All @@ -225,21 +219,29 @@ export const buildTeamId = id => `team:${id}`;
/**
* Removes the organization / project scope prefix on feature names.
*/
export function descopeFeatureName(feature) {
return typeof feature.match !== 'function'
? feature
: feature.match(/(?:^(?:projects|organizations):)?(.*)/).pop();
export function descopeFeatureName<T>(feature: T): T | string {
if (typeof feature !== 'string') {
return feature;
}

const results = feature.match(/(?:^(?:projects|organizations):)?(.*)/);

if (results && results.length > 0) {
return results.pop()!;
}

return feature;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@evanpurkhiser oh interesting. I'll revisit this.

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

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh, yeah makes sense.

}

export function isWebpackChunkLoadingError(error) {
export function isWebpackChunkLoadingError(error: Error): boolean {
return (
error &&
typeof error.message === 'string' &&
error.message.toLowerCase().includes('loading chunk')
);
}

export function deepFreeze(object) {
export function deepFreeze(object: {[x: string]: any}) {
// Retrieve the property names defined on object
const propNames = Object.getOwnPropertyNames(object);
// Freeze properties before freezing self
Expand Down