From 2751f400fdbb193d0138422fc9d9d93de1eb9e2b Mon Sep 17 00:00:00 2001 From: Felipe Cadavid Date: Thu, 15 Jun 2023 12:34:01 -0500 Subject: [PATCH] Fix Counter, Leaderboard and TimeSeries bugs (#73) * fix: fixes to TimeSeries component * fix: Leaderboard fixes * fix: counter fixes * fix: static mode bugs * refactor: remove unnecessary import * refactor: use ?? + remove unnecessary typeof * refactor: comment all console.error --- .../core/components/src/ErrorBoundary.tsx | 2 +- packages/core/release/src/index.ts | 2 +- packages/react/counter/src/Counter.tsx | 49 +++++++++++----- packages/react/counter/src/utils.ts | 6 +- .../react/leaderboard/src/Leaderboard.tsx | 56 +++++++++++-------- packages/react/time-series/src/TimeSeries.tsx | 37 +++++++++--- 6 files changed, 102 insertions(+), 50 deletions(-) diff --git a/packages/core/components/src/ErrorBoundary.tsx b/packages/core/components/src/ErrorBoundary.tsx index f984f50b..789b5bb3 100644 --- a/packages/core/components/src/ErrorBoundary.tsx +++ b/packages/core/components/src/ErrorBoundary.tsx @@ -22,7 +22,7 @@ export class ErrorBoundary extends Component { } componentDidCatch(error: Error, errorInfo: React.ErrorInfo) { - console.error(error, errorInfo) + // console.error(error, errorInfo) we will set logs as a feature later } render() { diff --git a/packages/core/release/src/index.ts b/packages/core/release/src/index.ts index 2bcfa490..1fd3e5b7 100644 --- a/packages/core/release/src/index.ts +++ b/packages/core/release/src/index.ts @@ -57,7 +57,7 @@ async function main(): Promise { } } } catch (error) { - console.error(error) + // console.error(error) we will set logs as a feature later process.exit(1) } } diff --git a/packages/react/counter/src/Counter.tsx b/packages/react/counter/src/Counter.tsx index 54fcf3cd..6fbc45da 100644 --- a/packages/react/counter/src/Counter.tsx +++ b/packages/react/counter/src/Counter.tsx @@ -55,6 +55,8 @@ export function Counter(props: CounterProps) { const [hasError, setHasError] = React.useState(false) const [isLoading, setIsLoading] = React.useState(false) + const filtersString = JSON.stringify(query?.filters || []) + /** * Fetches the counter data * when the user doesn't provide @@ -64,14 +66,21 @@ export function Counter(props: CounterProps) { try { setIsLoading(true) + const filters = JSON.parse(filtersString) + const response = await request( PROPEL_GRAPHQL_API_ENDPOINT, CounterDocument, { uniqueName: query?.metric, counterInput: { - timeRange: query?.timeRange, - filters: query?.filters, + timeRange: { + relative: query?.timeRange?.relative ?? null, + n: query?.timeRange?.n ?? null, + start: query?.timeRange?.start ?? null, + stop: query?.timeRange?.stop ?? null + }, + filters, propeller: query?.propeller } }, @@ -90,20 +99,29 @@ export function Counter(props: CounterProps) { } finally { setIsLoading(false) } - }, [query]) + }, [ + query?.metric, + query?.accessToken, + query?.timeRange?.n, + query?.timeRange?.relative, + query?.timeRange?.start, + query?.timeRange?.stop, + query?.propeller, + filtersString + ]) React.useEffect(() => { function handlePropsMismatch() { if (isStatic && !value) { - console.error('InvalidPropsError: You must pass either `value` or `query` props') + // console.error('InvalidPropsError: You must pass either `value` or `query` props') we will set logs as a feature later setHasError(true) return } if (!isStatic && (!query?.accessToken || !query?.metric || !query?.timeRange)) { - console.error( - 'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric` and `timeRange` in the `query` prop' - ) + // console.error( + // 'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric` and `timeRange` in the `query` prop' + // ) we will set logs as a feature later setHasError(true) return } @@ -118,18 +136,19 @@ export function Counter(props: CounterProps) { async function setup() { if (isStatic && value) { setDataValue(value) - return } - const fetchedValue = await fetchData() + if (!isStatic) { + const fetchedValue = await fetchData() - if (!fetchedValue) { - setHasError(true) - console.error(`QueryError: Your metric ${query?.metric} returned undefined or a \`null\` value.`) - return - } + if (fetchedValue === undefined) { + setHasError(true) + // console.error(`QueryError: Your metric ${query?.metric} returned undefined.`) we will set logs as a feature later + return + } - setDataValue(fetchedValue) + setDataValue(fetchedValue) + } } setup() diff --git a/packages/react/counter/src/utils.ts b/packages/react/counter/src/utils.ts index 63ea74a6..1a1b0b16 100644 --- a/packages/react/counter/src/utils.ts +++ b/packages/react/counter/src/utils.ts @@ -7,10 +7,14 @@ const getValue = (options: getValueOptions) => { const { value, localize } = options const isInteger = Number.isInteger(parseFloat(value)) + const isNull = value === null if (isInteger) { return localize ? parseInt(value).toLocaleString() : parseInt(value) } + if (isNull) { + return '-' + } return localize ? parseFloat(parseFloat(value).toFixed(2).toLocaleString()).toLocaleString() @@ -25,7 +29,7 @@ export const getValueWithPrefixAndSufix = (params: { }) => { const { prefix, value, sufix, localize } = params - if (!value) return + if (value === undefined) return return (prefix ? prefix : '') + getValue({ value, localize }) + (sufix ? sufix : '') } diff --git a/packages/react/leaderboard/src/Leaderboard.tsx b/packages/react/leaderboard/src/Leaderboard.tsx index 69ba3b5b..89e08c68 100644 --- a/packages/react/leaderboard/src/Leaderboard.tsx +++ b/packages/react/leaderboard/src/Leaderboard.tsx @@ -88,6 +88,9 @@ export function Leaderboard(props: LeaderboardProps) { const idRef = React.useRef(idCounter++) const id = `leaderboard-${idRef.current}` + const filtersString = JSON.stringify(query?.filters || []) + const dimensionsString = JSON.stringify(query?.dimensions || []) + /** * The html node where the chart will render */ @@ -196,18 +199,26 @@ export function Leaderboard(props: LeaderboardProps) { try { setIsLoading(true) + const dimensions = JSON.parse(dimensionsString) + const filters = JSON.parse(filtersString) + const response = await request( PROPEL_GRAPHQL_API_ENDPOINT, LeaderboardDocument, { uniqueName: query?.metric, leaderboardInput: { - timeRange: query?.timeRange, - filters: query?.filters, + filters, propeller: query?.propeller, sort: query?.sort, rowLimit: query?.rowLimit, - dimensions: query?.dimensions + dimensions, + timeRange: { + relative: query?.timeRange?.relative ?? null, + n: query?.timeRange?.n ?? null, + start: query?.timeRange?.start ?? null, + stop: query?.timeRange?.stop ?? null + } } }, { @@ -228,25 +239,28 @@ export function Leaderboard(props: LeaderboardProps) { } }, [ query?.accessToken, - query?.dimensions, - query?.filters, + dimensionsString, + filtersString, query?.metric, query?.propeller, query?.rowLimit, query?.sort, - query?.timeRange + query?.timeRange?.n, + query?.timeRange?.relative, + query?.timeRange?.start, + query?.timeRange?.stop ]) React.useEffect(() => { function handlePropsMismatch() { if (isStatic && !headers && !rows) { - console.error('InvalidPropsError: You must pass either `headers` and `rows` or `query` props') + // console.error('InvalidPropsError: You must pass either `headers` and `rows` or `query` props') we will set logs as a feature later setHasError(true) return } if (isStatic && (!headers || !rows)) { - console.error('InvalidPropsError: When passing the data via props you must pass both `headers` and `rows`') + // console.error('InvalidPropsError: When passing the data via props you must pass both `headers` and `rows`') we will set logs as a feature later setHasError(true) return } @@ -254,9 +268,9 @@ export function Leaderboard(props: LeaderboardProps) { !isStatic && (!query.accessToken || !query.metric || !query.timeRange || !query.dimensions || !query.rowLimit) ) { - console.error( - 'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric`, `dimensions`, `rowLimit` and `timeRange` in the `query` prop' - ) + // console.error( + // 'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric`, `dimensions`, `rowLimit` and `timeRange` in the `query` prop' + // ) we will set logs as a feature later setHasError(true) return } @@ -275,17 +289,7 @@ export function Leaderboard(props: LeaderboardProps) { if (!isStatic) { fetchChartData() } - }, [ - isStatic, - query?.timeRange, - query?.filters, - query?.propeller, - query?.sort, - query?.rowLimit, - query?.dimensions, - query?.rowLimit, - fetchData - ]) + }, [isStatic, fetchData]) React.useEffect(() => { if (isStatic) { @@ -302,7 +306,7 @@ export function Leaderboard(props: LeaderboardProps) { React.useEffect(() => { try { if (variant !== 'bar' && variant !== 'table') { - console.error('InvalidPropsError: `variant` prop must be either `bar` or `table`') + // console.error('InvalidPropsError: `variant` prop must be either `bar` or `table`') we will set logs as a feature later throw new Error('InvalidPropsError') } setHasError(false) @@ -317,6 +321,12 @@ export function Leaderboard(props: LeaderboardProps) { } }, []) + React.useEffect(() => { + if (variant === 'table') { + destroyChart() + } + }, [variant]) + if (isLoading || loading) { destroyChart() return diff --git a/packages/react/time-series/src/TimeSeries.tsx b/packages/react/time-series/src/TimeSeries.tsx index c47e814e..031508fe 100644 --- a/packages/react/time-series/src/TimeSeries.tsx +++ b/packages/react/time-series/src/TimeSeries.tsx @@ -129,6 +129,8 @@ export function TimeSeries(props: TimeSeriesProps) { const idRef = React.useRef(idCounter++) const id = `time-series-${idRef.current}` + const filtersString = JSON.stringify(query?.filters || []) + /** * The html node where the chart will render */ @@ -258,15 +260,22 @@ export function TimeSeries(props: TimeSeriesProps) { try { setIsLoading(true) + const filters = JSON.parse(filtersString) + const response = await request( PROPEL_GRAPHQL_API_ENDPOINT, TimeSeriesDocument, { uniqueName: query?.metric, timeSeriesInput: { - timeRange: query?.timeRange, + timeRange: { + relative: query?.timeRange?.relative ?? null, + n: query?.timeRange?.n ?? null, + start: query?.timeRange?.start ?? null, + stop: query?.timeRange?.stop ?? null + }, granularity, - filters: query?.filters, + filters: filters, propeller: query?.propeller } }, @@ -286,25 +295,35 @@ export function TimeSeries(props: TimeSeriesProps) { } finally { setIsLoading(false) } - }, [granularity, query?.accessToken, query?.filters, query?.metric, query?.propeller, query?.timeRange]) + }, [ + granularity, + query?.accessToken, + filtersString, + query?.metric, + query?.propeller, + query?.timeRange?.n, + query?.timeRange?.relative, + query?.timeRange?.start, + query?.timeRange?.stop + ]) React.useEffect(() => { function handlePropsMismatch() { if (isStatic && !labels && !values) { - console.error('InvalidPropsError: You must pass either `labels` and `values` or `query` props') + // console.error('InvalidPropsError: You must pass either `labels` and `values` or `query` props') we will set logs as a feature later setHasError(true) return } if (isStatic && (!labels || !values)) { - console.error('InvalidPropsError: When passing the data via props you must pass both `labels` and `values`') + // console.error('InvalidPropsError: When passing the data via props you must pass both `labels` and `values`') we will set logs as a feature later setHasError(true) return } if (!isStatic && (!query.accessToken || !query.metric || !query.timeRange)) { - console.error( - 'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric` and `timeRange` in the `query` prop' - ) + // console.error( + // 'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric` and `timeRange` in the `query` prop' + // ) we will set logs as a feature later setHasError(true) return } @@ -324,7 +343,7 @@ export function TimeSeries(props: TimeSeriesProps) { if (!isStatic) { fetchChartData() } - }, [isStatic, query?.timeRange, query?.filters, query?.propeller, query?.granularity, query?.accessToken, fetchData]) + }, [isStatic, fetchData]) React.useEffect(() => { if (isStatic) {