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/09/22 18:26:12 UTC
[GitHub] [incubator-superset] graceguo-supercat opened a new pull request #11002: fix: Revert #10455
graceguo-supercat opened a new pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002
### SUMMARY
Please see discussion in https://github.com/apache/incubator-superset/issues/10849.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: #10455
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
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] graceguo-supercat commented on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696915309
Please see Superset revert guidelines here: https://github.com/apache/incubator-superset/blob/master/CONTRIBUTING.md#revert-guidelines
This issue has pretty big impact but has small size of changes. The root cause is unknown for the owner, so I assume this will take long time to get fixed.
----------------------------------------------------------------
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 edited a comment on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696999553
> @tanmaylaud I'm not sure "functional components is recommended" is really a statement that can be broadly applied to all existing code.
Just commenting on this one. Functional components [are absolutely the future of React](https://blog.bitsrc.io/will-react-classes-get-deprecated-because-of-hooks-bb62938ac1f5). I understand it requires significant amount of work to refactor some existing components, but I'd recommend everyone to start writing ALL new component as functional components and convert old components when possible (See the linked article for reasoning.)
We should probably even add "new components should be written as functional components" to the contribution guideline.
----------------------------------------------------------------
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 edited a comment on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
tanmaylaud edited a comment on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696915485
----------------------------------------------------------------
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 edited a comment on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-697012273
I agree for the most part. Functional components + hooks work quite differently than class based components. For new components it absolutely makes sense to _prefer_ functional components, especially for new components. I just wanted to point out that refactoring class components to be function components requires rethinking the entire implementation and can result in unexpected bugs that are difficult to diagnose. It is a significantly more complex task than say converting a component from jsx to tsx.
----------------------------------------------------------------
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] graceguo-supercat closed pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
graceguo-supercat closed pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-697137570
+1 on @nytai 's points. We have to find a balance between switching the latest/greatest trend and sticking with patterns that work and are consistent. So many bandwagons and reasons to "rewrite all the things".
I don't see anything wrong with a nice class component that works nicely. If it's not broken don't fix it!
----------------------------------------------------------------
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] graceguo-supercat commented on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696905441
I feel `functional components` is not the root cause. Maybe the implementation for this component has bug?
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696948384
fyi, i think https://github.com/apache/incubator-superset/pull/11003 will unbreak CI. Not sure what the problem is, but this unbreaks it for 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] tanmaylaud edited a comment on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
tanmaylaud edited a comment on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696915485
> @tanmaylaud I'm not sure "functional components is recommended" is really a statement that can be broadly applied to all existing code. Functional components + hooks work significantly different than class components, so there is a non-trivial refactor needed to reimplement the existing logic using hooks. As with all substantial refactors, substantial testing is required to ensure functionality has not changed. I see you removed some tests in that PR. Looks like a test case that verifies the timer is reset is missing.
@nytai I am not denying that there is some issue with the current implementation but I am only asking that we forward fix the issue.
The test cases were changed(not removed) . I didn't do it randomly. There are limitations on how the useEffect hooks can be tested. It would be great if you can let me know the issue in the current code. I think for now, we can revert the code as desired. I will try to analyse the issue in the functional logic
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696997987
rerunning with new CI config
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
tanmaylaud commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696904190
----------------------------------------------------------------
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] graceguo-supercat closed pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
graceguo-supercat closed pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002
----------------------------------------------------------------
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 edited a comment on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696999553
> @tanmaylaud I'm not sure "functional components is recommended" is really a statement that can be broadly applied to all existing code.
Just commenting on this one. Functional components [are absolutely the future of React](https://blog.bitsrc.io/will-react-classes-get-deprecated-because-of-hooks-bb62938ac1f5). I understand it requires significant amount of work to refactor some existing components, but I'd recommend everyone to start writing ALL new component as functional components and convert old components when possible (See the linked article for reasoning.)
We should probably even add "new components should be written as functional components" to the contribution guideline.
----------------------------------------------------------------
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 pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696999553
> @tanmaylaud I'm not sure "functional components is recommended" is really a statement that can be broadly applied to all existing code.
Just commenting on this one. Functional components [are absolutely the future of React](https://blog.bitsrc.io/will-react-classes-get-deprecated-because-of-hooks-bb62938ac1f5). I understand it requires significant amount of work to refactor some existing components, but I'd recommend everyone to start writing ALL new component as functional components and convert old components to functional when possible (See the linked article for reasoning.)
We should probably even add "new components should be written as functional components" to the contribution guideline.
----------------------------------------------------------------
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 pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-697154594
> I agree for the most part. Functional components + hooks work quite differently than class based components. For new components it absolutely makes sense to _prefer_ functional components, especially for new components. I just wanted to point out that refactoring class components to be function components requires rethinking the entire implementation and can result in unexpected bugs that are difficult to diagnose. It is a significantly more complex task than say converting a component from jsx to tsx.
100% agree. I'd also like to point out sometimes converting to TypeScript is also not as easy as one might think. It's one thing to change a file name and add a bunch of primitive types, it's another to add proper strong typing that really gives future developers peace of mind and smart intellisense.
I'm all in favor of stability and not doing refactoring for refactoring's sake. But if you can improve code quality along the way and are confident about your changes, by all means please do it. It'll be a win for everyone in the long run.
I think right now it's just people are still not used to the best practices and common patterns with React hooks, so things can be a little brittle when hooks are used incorrectly. Enabling [the react/hooks ESLint rule](https://github.com/apache/incubator-superset/pull/11006) should help mitigate this more or less.
----------------------------------------------------------------
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 edited a comment on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-697012273
I agree for the most part. Functional components + hooks work quite differently than class based components. For new components it absolutely makes sense to _prefer_ functional components, especially for new components. I just wanted to point out that refactoring class components to be function components requires rethinking the entire implementation and can result in unexpected bugs that are difficult to diagnose. It is a significantly more complex task than say converting a component from jsx to tsx.
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
tanmaylaud commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696907880
> I feel `functional components` is not the root cause. Maybe the implementation for this component has bug?
Exactly ! That's why I am asking to figure out the bug in the current code rather than reverting to older class component
----------------------------------------------------------------
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] graceguo-supercat commented on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696905441
----------------------------------------------------------------
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] graceguo-supercat commented on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-697016831
Thanks for @ktmud quick fix. I will close this PR and favor the new solution!
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696910033
@tanmaylaud I'm not sure "functional components is recommended" is really a statement that can be broadly applied to all existing code. Functional components + hooks work significantly different than class components, so there is a non-trivial refactor needed to reimplement the existing logic using hooks. As with all substantial refactors, substantial testing is required to ensure functionality has not changed. I see you removed some tests in that PR. Looks like a test case that verifies the timer is reset is missing.
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696910033
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-697012273
I agree for the most part. Functional components + hooks work quite differently than class based components. For new components it absolutely makes sense to prefer functional components. I just wanted to point out that refactoring class components to be function components requires rethinking the entire implementation and can result in unexpected bugs that are difficult to diagnose. It is a significantly more complex task than say converting a component from jsx to tsx.
----------------------------------------------------------------
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 pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696999553
> @tanmaylaud I'm not sure "functional components is recommended" is really a statement that can be broadly applied to all existing code.
Just commenting on this one. Functional components [are absolutely the future of React](https://blog.bitsrc.io/will-react-classes-get-deprecated-because-of-hooks-bb62938ac1f5). I understand it requires significant amount of work to refactor some existing components, but I'd recommend everyone to start writing ALL new component as functional components and convert old components to functional when possible (See the linked article for reasoning.)
We should probably even add "new components should be written as functional components" to the contribution guideline.
----------------------------------------------------------------
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 closed pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
nytai closed pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002
----------------------------------------------------------------
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 edited a comment on pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
tanmaylaud edited a comment on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696915485
> @tanmaylaud I'm not sure "functional components is recommended" is really a statement that can be broadly applied to all existing code. Functional components + hooks work significantly different than class components, so there is a non-trivial refactor needed to reimplement the existing logic using hooks. As with all substantial refactors, substantial testing is required to ensure functionality has not changed. I see you removed some tests in that PR. Looks like a test case that verifies the timer is reset is missing.
I am not denying that there is some issue with the current implementation but I am only asking that we forward fix the issue.
The test cases were changed(not removed) . I didn't do it randomly. There are limitations on how the useEffect hooks can be tested. It would be great if you can let me know the issue in the current code. I think for now, we can revert the code as desired. I will try to analyse the issue in the functional logic
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
tanmaylaud commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696904190
thanks @graceguo-supercat for reverting.
Although it would be important to discuss and figure out what is exactly breaking the Timer logic.
Use of functional components is recommended, so simply reverting to class components will not be useful in the long run.
Let me know if we can open up a discussion for this? @JunlinC @etr2460
----------------------------------------------------------------
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 closed pull request #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
nytai closed pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
tanmaylaud commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696915485
> @tanmaylaud I'm not sure "functional components is recommended" is really a statement that can be broadly applied to all existing code. Functional components + hooks work significantly different than class components, so there is a non-trivial refactor needed to reimplement the existing logic using hooks. As with all substantial refactors, substantial testing is required to ensure functionality has not changed. I see you removed some tests in that PR. Looks like a test case that verifies the timer is reset is missing.
I am not denying that there is some issue with the current implementation but I am only asking that we forward fix the issue.
The test cases were changed(not removed) . I didn't do it randomly. There are limitations on how the useEffect hooks can be tested. It would be great if you can let me know the issue in the current code.
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696942671
@tanmaylaud Absolutely, but for now reverting is the quickest path to restoring user functionality. I agree, testing hooks can be quite challenging. I've done some digging and I have a few thoughts as to what is likely happening. I'll add some comments to the original PR.
----------------------------------------------------------------
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 #11002: fix: revert #10455 to fix the Timer component
Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #11002:
URL: https://github.com/apache/incubator-superset/pull/11002#issuecomment-696948384
fyi, i think https://github.com/apache/incubator-superset/pull/11003 will unbreak CI. Not sure what the problem is, but this unbreaks it for 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