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

Jump to bottom

refactor(frontend): move utils to TypeScript #9820

Merged
merged 3 commits into from
May 20, 2020
Merged

refactor(frontend): move utils to TypeScript #9820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d43fe7e
refactor(frontend): move utils to typescript (#9101)
ChristianMurphy May 16, 2020
bf6faac
refactor(frontend): don't export interfaces
ChristianMurphy May 17, 2020
4ddab59
test(frontend): update types and test for isValidChild
ChristianMurphy May 20, 2020
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
0 ...shboard/util/componentIsResizable_spec.js → ...shboard/util/componentIsResizable_spec.ts
View file
Open in desktop
File renamed without changes.
12 changes: 7 additions & 5 deletions ...ripts/dashboard/util/isValidChild_spec.js → ...ripts/dashboard/util/isValidChild_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
TAB_TYPE as TAB,
} from 'src/dashboard/util/componentTypes';

const getIndentation = depth =>
const getIndentation = (depth: number) =>
Array(depth * 3)
.fill('')
.join('-');
Expand Down Expand Up @@ -136,20 +136,22 @@ describe('isValidChild', () => {
invalidExamples.forEach((example, exampleIdx) => {
let childDepth = 0;
example.forEach((childType, i) => {
const shouldTestChild = Array.isArray(childType);

if (i > 0 && shouldTestChild) {
// should test child
if (i > 0 && Array.isArray(childType)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently type narrowing using Array.isArray is limited to when the it is inside an if condition

const parentDepth = childDepth - 1;
const parentType = example[i - 1];

if (typeof parentType !== 'string')
throw TypeError('parent must be string');
Comment on lines +144 to +145
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type narrowing needed due to mixed string and string[] in the array, this should never throw


it(`(${exampleIdx})${getIndentation(
childDepth,
)}${parentType} (depth ${parentDepth}) > ${childType} ❌`, () => {
expect(
isValidChild({
parentDepth,
parentType,
childType,
childType: childType[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.

this wasn't quite testing what it said it was testing, it was seeing if an array matches the string literal at a given level.
This fixes the tests to compare strings to strings.

}),
).toBe(false);
});
Expand Down
0 .../dashboard/util/backgroundStyleOptions.js → .../dashboard/util/backgroundStyleOptions.ts
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion ...rc/dashboard/util/componentIsResizable.js → ...rc/dashboard/util/componentIsResizable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@
*/
import { COLUMN_TYPE, CHART_TYPE, MARKDOWN_TYPE } from './componentTypes';

export default function componentIsResizable(entity) {
export default function componentIsResizable(entity: { type: string }) {
return [COLUMN_TYPE, CHART_TYPE, MARKDOWN_TYPE].indexOf(entity.type) > -1;
}
0 ...tend/src/dashboard/util/componentTypes.js → ...tend/src/dashboard/util/componentTypes.ts
View file
Open in desktop
File renamed without changes.
0 ...-frontend/src/dashboard/util/constants.js → ...-frontend/src/dashboard/util/constants.ts
View file
Open in desktop
File renamed without changes.
13 changes: 11 additions & 2 deletions ...c/dashboard/util/getDashboardFilterKey.js → ...c/dashboard/util/getDashboardFilterKey.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,20 @@
* specific language governing permissions and limitations
* under the License.
*/
export function getDashboardFilterKey({ chartId, column }) {

interface GetDashboardFilterKeyProps {
chartId: string;
column: string;
}

export function getDashboardFilterKey({
chartId,
column,
}: GetDashboardFilterKeyProps) {
return `${chartId}_${column}`;
}

export function getChartIdAndColumnFromFilterKey(key) {
export function getChartIdAndColumnFromFilterKey(key: string) {
const [chartId, ...parts] = key.split('_');
const column = parts.slice().join('_');
return { chartId: parseInt(chartId, 10), column };
Expand Down
0 ...end/src/dashboard/util/getLocationHash.js → ...end/src/dashboard/util/getLocationHash.ts
View file
Open in desktop
File renamed without changes.
31 changes: 22 additions & 9 deletions .../dashboard/util/getRevertedFilterScope.js → .../dashboard/util/getRevertedFilterScope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,33 @@
*/
import { getChartIdAndColumnFromFilterKey } from './getDashboardFilterKey';

interface FilterScopeMap {
[key: string]: number[];
}

interface GetRevertFilterScopeProps {
checked: string[];
filterFields: string[];
filterScopeMap: FilterScopeMap;
}

export default function getRevertedFilterScope({
checked = [],
filterFields = [],
filterScopeMap = {},
}) {
const checkedChartIdsByFilterField = checked.reduce((map, value) => {
const [chartId, filterField] = value.split(':');
return {
...map,
[filterField]: (map[filterField] || []).concat(parseInt(chartId, 10)),
};
}, {});
}: GetRevertFilterScopeProps) {
const checkedChartIdsByFilterField = checked.reduce<FilterScopeMap>(
(map, value) => {
const [chartId, filterField] = value.split(':');
return {
...map,
[filterField]: (map[filterField] || []).concat(parseInt(chartId, 10)),
};
},
{},
);

return filterFields.reduce((map, filterField) => {
return filterFields.reduce<FilterScopeMap>((map, filterField) => {
const { chartId } = getChartIdAndColumnFromFilterKey(filterField);
// force display filter_box chart as unchecked, but show checkbox as disabled
const updatedCheckedIds = (
Expand Down
0 .../src/dashboard/util/headerStyleOptions.js → .../src/dashboard/util/headerStyleOptions.ts
View file
Open in desktop
File renamed without changes.
13 changes: 11 additions & 2 deletions ...ontend/src/dashboard/util/isValidChild.js → ...ontend/src/dashboard/util/isValidChild.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,21 @@ const parentMaxDepthLookup = {
[MARKDOWN_TYPE]: {},
};

export default function isValidChild({ parentType, childType, parentDepth }) {
interface IsValidChildProps {
parentType?: string;
childType?: string;
parentDepth?: unknown;
}

export default function isValidChild(child: IsValidChildProps): boolean {
const { parentType, childType, parentDepth } = child;
if (!parentType || !childType || typeof parentDepth !== 'number') {
Copy link

@graceguo-supercat graceguo-supercat May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

since we use TypeScript, is type check here still necessary?

Copy link
Contributor Author

@ChristianMurphy ChristianMurphy May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It depends, I aimed to keep the behavior the same, taking in an unknown and returning a known type.

This entire function could theoretically be completely removed, using a combo of keyof and generics can statically check most of this:

interface ValidChild<
  P extends keyof ParentMaxDepthLookup,
  C extends keyof ParentMaxDepthLookup[P],
  D extends ParentMaxDepthLookup[P][C]
> {
  parentType: P;
  childType: C;
  parentDepth: D;
}

as an intermediate step, I could set parentDepth to be number and remove this check.

thoughts? 💭

Copy link

Choose a reason for hiding this comment

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

yes, i feel parentDepth should be number type. This intermediate step a little confused me. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@graceguo-supercat I removed the generics which were causing confusion.
The request to make parentDepth a number and remove the typeof check causes a cascade affect where two other test suites fail

Test Suites: 2 failed, 186 passed, 188 total
Tests:       17 failed, 4 skipped, 1040 passed, 1061 total
Snapshots:   12 passed, 12 total
Time:        67.338s, estimated 78s

specifically getDropPosition and isValidChild both expect the number check.

return false;
}

const maxParentDepth = (parentMaxDepthLookup[parentType] || {})[childType];
const maxParentDepth: number | undefined = (parentMaxDepthLookup[
parentType
] || {})[childType];

return typeof maxParentDepth === 'number' && parentDepth <= maxParentDepth;
}
0 ...end/src/dashboard/util/resizableConfig.js → ...end/src/dashboard/util/resizableConfig.ts
View file
Open in desktop
File renamed without changes.
10 changes: 8 additions & 2 deletions ...d/src/dashboard/util/setPeriodicRunner.js → ...d/src/dashboard/util/setPeriodicRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,23 @@
* specific language governing permissions and limitations
* under the License.
*/
const stopPeriodicRender = refreshTimer => {
const stopPeriodicRender = (refreshTimer?: number) => {
if (refreshTimer) {
clearInterval(refreshTimer);
}
};

interface SetPeriodicRunnerProps {
interval?: number;
periodicRender: TimerHandler;
refreshTimer?: number;
}

export default function setPeriodicRunner({
interval = 0,
periodicRender,
refreshTimer,
}) {
}: SetPeriodicRunnerProps) {
stopPeriodicRender(refreshTimer);

if (interval > 0) {
Expand Down