You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/07/29 07:25:03 UTC
[GitHub] [incubator-superset] tanmaylaud opened a new pull request #10455: chore: Migrate Timer component from jsx to tsx
tanmaylaud opened a new pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455
### SUMMARY
Migrated the Timer component as part of #10004
### TEST PLAN
Timer in SQLEditor still works as expected when a query is fired
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#discussion_r462639063
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: object;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+ style,
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState<string>('');
Review comment:
if we're defaulting this to empty string, then we shouldn't need to specify `<string>`
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: object;
Review comment:
A type should exist for styles here. possibly `React.CSSProperties`, although it might be something else (maybe from `react-bootstrap`. Let's see if we can use a more specific type
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: object;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+ style,
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState<string>('');
+ const timer = useRef<NodeJS.Timeout>();
+
+ const stopwatch = () => {
+ if (startTime) {
+ const endDttm = endTime || now();
+ if (startTime < endDttm) {
+ setClockStr(fDuration(startTime, endDttm));
+ }
+ if (!isRunning) {
+ clearTimeout(timer.current!);
+ }
+ }
+ };
+
+ function stopTimer() {
+ clearInterval(timer.current!);
Review comment:
if we get rid of the ref, then we wouldn't need the `!` here either (which would be a bit more type safe).
Otherwise, it might be better to wrap this in an `if` statement anyway, instead of ignoring the types
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: object;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+ style,
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState<string>('');
+ const timer = useRef<NodeJS.Timeout>();
+
+ const stopwatch = () => {
+ if (startTime) {
+ const endDttm = endTime || now();
+ if (startTime < endDttm) {
+ setClockStr(fDuration(startTime, endDttm));
+ }
+ if (!isRunning) {
+ clearTimeout(timer.current!);
+ }
+ }
+ };
+
+ function stopTimer() {
Review comment:
we're defining functions here in 2 different ways:
`const stopwatch = () => {`
and
`function stopTimer() {`
Personally I prefer the former for inline function definitions, but consistency is more important to me
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: object;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+ style,
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState<string>('');
+ const timer = useRef<NodeJS.Timeout>();
Review comment:
any reason not to use state for this instead of a ref? The component should be rerendering super often anyway, so i don't think it should have perf implications
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] tanmaylaud commented on a change in pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
tanmaylaud commented on a change in pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#discussion_r462762720
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: object;
Review comment:
react-bootstrap has the 'bsStyle' . So, putting React.CSSProperties makes sense. Adding it.
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: object;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+ style,
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState<string>('');
Review comment:
Yes, it is not required. removing it.
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: object;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+ style,
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState<string>('');
+ const timer = useRef<NodeJS.Timeout>();
Review comment:
I was waiting for someone to look at this from performance POV. Thanks for this. will change it to useState. I had put useRef considering the timer was to be set and unset very often. But useState makes it simpler.
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: object;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+ style,
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState<string>('');
+ const timer = useRef<NodeJS.Timeout>();
+
+ const stopwatch = () => {
+ if (startTime) {
+ const endDttm = endTime || now();
+ if (startTime < endDttm) {
+ setClockStr(fDuration(startTime, endDttm));
+ }
+ if (!isRunning) {
+ clearTimeout(timer.current!);
+ }
+ }
+ };
+
+ function stopTimer() {
+ clearInterval(timer.current!);
Review comment:
Agreed!
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: object;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+ style,
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState<string>('');
+ const timer = useRef<NodeJS.Timeout>();
+
+ const stopwatch = () => {
+ if (startTime) {
+ const endDttm = endTime || now();
+ if (startTime < endDttm) {
+ setClockStr(fDuration(startTime, endDttm));
+ }
+ if (!isRunning) {
+ clearTimeout(timer.current!);
+ }
+ }
+ };
+
+ function stopTimer() {
Review comment:
defaulting to arrow notation đź‘Ť
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai commented on a change in pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#discussion_r492997908
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState('');
+ const [timer, setTimer] = useState<NodeJS.Timeout>();
+
+ const stopTimer = () => {
+ if (timer) {
+ clearInterval(timer);
+ setTimer(undefined);
+ }
+ };
+
+ const stopwatch = () => {
+ if (startTime) {
+ const endDttm = endTime || now();
+ if (startTime < endDttm) {
+ setClockStr(fDuration(startTime, endDttm));
+ }
+ if (!isRunning) {
+ stopTimer();
+ }
+ }
+ };
+
+ const startTimer = () => {
+ setTimer(setInterval(stopwatch, 30));
+ };
+
+ useEffect(() => {
+ if (isRunning) {
+ startTimer();
+ }
+ }, [isRunning]);
+
+ useEffect(() => {
+ return () => {
+ stopTimer();
+ };
+ });
Review comment:
From the react docs
> When exactly does React clean up an effect? React performs the cleanup when the component unmounts. However, as we learned earlier, effects run for every render and not just once. This is why React also cleans up effects from the previous render before running the effects next time. We’ll discuss why this helps avoid bugs and how to opt out of this behavior in case it creates performance issues later below.
Looks like the issue here is that this is running on every render, hence it appears to call stopTimer after a single update. A simple way to handle this is to add a dependency on the prop that signals the query has started/stopped running, namely `endTime`.
Suggested change
```js
useEffect(() => {
return () => {
stopTimer();
};
}, [endTime]);
```
A test case to ensure this doesn't break in future refactors would great.
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState('');
+ const [timer, setTimer] = useState<NodeJS.Timeout>();
+
+ const stopTimer = () => {
+ if (timer) {
+ clearInterval(timer);
+ setTimer(undefined);
+ }
+ };
+
+ const stopwatch = () => {
+ if (startTime) {
+ const endDttm = endTime || now();
+ if (startTime < endDttm) {
+ setClockStr(fDuration(startTime, endDttm));
+ }
+ if (!isRunning) {
+ stopTimer();
+ }
+ }
+ };
+
+ const startTimer = () => {
+ setTimer(setInterval(stopwatch, 30));
+ };
+
+ useEffect(() => {
+ if (isRunning) {
+ startTimer();
+ }
+ }, [isRunning]);
+
+ useEffect(() => {
+ return () => {
+ stopTimer();
+ };
+ });
Review comment:
actually, nevermind. This doesn't work for cases where the query errors out
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai commented on pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#issuecomment-665248018
@tanmaylaud Looks like uni tests are failing for that component, they likely need to be rewritten to account of the use of hooks instead of class component state.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] tanmaylaud commented on pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
tanmaylaud commented on pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#issuecomment-670166189
> @tanmaylaud , I tried to fix CI, but it looks like the tests in `Timer_spec.jsx` run forever, causing the failure. Maybe you can look into this?
@etr2460 My bad. Fixed it now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#issuecomment-670326100
@etr2460 when using a simple `mv` (or renaming in an IDE for instance), the `jsx` file will show as deleted and the `tsx` file as new. When using `git mv`, the history of the file stays attached to it.
This is my similar PR that keeps the history: https://github.com/apache/incubator-superset/pull/10529/files
Not a huge deal, but generally it's better to keep the history. I do more git archeology than I should (trying to figure out how a certain line of code or section came to be) and I'll run `git blame` or look at Github's history pages for a given file, and it's nice to keep history attached when possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#discussion_r463118658
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: React.CSSProperties;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+ style,
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState('');
+ const [timer, setTimer] = useState<NodeJS.Timeout>();
+
+ const stopTimer = () => {
+ if (timer) clearInterval(timer);
Review comment:
Do you think we should set `timer` back to null or undefined here? Not sure if it matters, but it might be neater
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#issuecomment-665430974
Let's use `git mv` to keep the file's change history, otherwise we loose all the info, otherwise LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai commented on a change in pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#discussion_r493000150
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState('');
+ const [timer, setTimer] = useState<NodeJS.Timeout>();
+
+ const stopTimer = () => {
+ if (timer) {
+ clearInterval(timer);
+ setTimer(undefined);
+ }
+ };
+
+ const stopwatch = () => {
+ if (startTime) {
+ const endDttm = endTime || now();
+ if (startTime < endDttm) {
+ setClockStr(fDuration(startTime, endDttm));
+ }
+ if (!isRunning) {
+ stopTimer();
+ }
+ }
+ };
+
+ const startTimer = () => {
+ setTimer(setInterval(stopwatch, 30));
+ };
+
+ useEffect(() => {
+ if (isRunning) {
+ startTimer();
+ }
+ }, [isRunning]);
+
+ useEffect(() => {
+ return () => {
+ stopTimer();
+ };
+ });
Review comment:
actually, nevermind. This doesn't work for cases where the query errors out
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] tanmaylaud commented on a change in pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
tanmaylaud commented on a change in pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#discussion_r463156607
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+ style?: React.CSSProperties;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+ style,
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState('');
+ const [timer, setTimer] = useState<NodeJS.Timeout>();
+
+ const stopTimer = () => {
+ if (timer) clearInterval(timer);
Review comment:
yeah.. I think react would do that. But adding that explicitly for better cleanup
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch edited a comment on pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
mistercrunch edited a comment on pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#issuecomment-670326100
@etr2460 when using a simple `mv` (or renaming in an IDE for instance), the `jsx` file will show as deleted and the `tsx` file as new. When using `git mv`, the history of the file stays attached to it.
This is my similar PR that keeps the history: https://github.com/apache/incubator-superset/pull/10529/files
Not a huge deal, but generally it's better to keep the history. I do more git archeology than I should (trying to figure out how a certain line of code or section came to be) and I'll run `git blame`, sometimes multiple times through layers of edits, or look at Github's history pages for a given file, and it's nice to keep history attached when possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai commented on a change in pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#discussion_r492997908
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
+
+interface TimerProps {
+ endTime?: number;
+ isRunning: boolean;
+ startTime?: number;
+ status?: string;
+}
+
+export default function Timer({
+ endTime,
+ isRunning,
+ startTime,
+ status = 'success',
+}: TimerProps) {
+ const [clockStr, setClockStr] = useState('');
+ const [timer, setTimer] = useState<NodeJS.Timeout>();
+
+ const stopTimer = () => {
+ if (timer) {
+ clearInterval(timer);
+ setTimer(undefined);
+ }
+ };
+
+ const stopwatch = () => {
+ if (startTime) {
+ const endDttm = endTime || now();
+ if (startTime < endDttm) {
+ setClockStr(fDuration(startTime, endDttm));
+ }
+ if (!isRunning) {
+ stopTimer();
+ }
+ }
+ };
+
+ const startTimer = () => {
+ setTimer(setInterval(stopwatch, 30));
+ };
+
+ useEffect(() => {
+ if (isRunning) {
+ startTimer();
+ }
+ }, [isRunning]);
+
+ useEffect(() => {
+ return () => {
+ stopTimer();
+ };
+ });
Review comment:
From the react docs
> When exactly does React clean up an effect? React performs the cleanup when the component unmounts. However, as we learned earlier, effects run for every render and not just once. This is why React also cleans up effects from the previous render before running the effects next time. We’ll discuss why this helps avoid bugs and how to opt out of this behavior in case it creates performance issues later below.
Looks like the issue here is that this is running on every render, hence it appears to call stopTimer after a single update. A simple way to handle this is to add a dependency on the prop that signals the query has started/stopped running, namely `endTime`.
Suggested change
```js
useEffect(() => {
return () => {
stopTimer();
};
}, [endTime]);
```
A test case to ensure this doesn't break in future refactors would great.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 merged pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
etr2460 merged pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#discussion_r493026991
##########
File path: superset-frontend/src/components/Timer.tsx
##########
@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useState } from 'react';
+import { Label } from 'react-bootstrap';
+
+import { now, fDuration } from '../modules/dates';
Review comment:
Please use absolute import `import .. from 'src/modules/dates'` in the future, it's easier to read (especially when multiple folders have files of the same name) and makes moving files around easier.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] tanmaylaud commented on pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
tanmaylaud commented on pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#issuecomment-665227220
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on pull request #10455: chore: Migrate Timer component from jsx to tsx
Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #10455:
URL: https://github.com/apache/incubator-superset/pull/10455#issuecomment-669506796
@tanmaylaud , I tried to fix CI, but it looks like the tests in `Timer_spec.jsx` run forever, causing the failure. Maybe you can look into this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org