Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: revert #10455 to fix the Timer component #11002

Closed
wants to merge 2 commits into from
Closed

fix: revert #10455 to fix the Timer component #11002

wants to merge 2 commits into from

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Sep 22, 2020 •
edited
Loading

SUMMARY

Please see discussion in #10849.

ADDITIONAL INFORMATION

cc @zuzana-vej @mistercrunch @tanmaylaud

@etr2460 etr2460 changed the title fix: Revert #10455 fix: revert #10455 to fix the Timer component Sep 22, 2020
@tanmaylaud
Copy link
Contributor

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

@graceguo-supercat
Copy link
Author

I feel functional components is not the root cause. Maybe the implementation for this component has bug?

@tanmaylaud
Copy link
Contributor

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

@nytai
Copy link
Member

nytai commented Sep 22, 2020

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

@graceguo-supercat
Copy link
Author

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.

@tanmaylaud
Copy link
Contributor

tanmaylaud commented Sep 22, 2020 •
edited
Loading

@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

@nytai
Copy link
Member

nytai commented Sep 22, 2020

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

@etr2460
Copy link
Member

etr2460 commented Sep 22, 2020

fyi, i think #11003 will unbreak CI. Not sure what the problem is, but this unbreaks it for now

@ktmud ktmud mentioned this pull request Sep 22, 2020
Merged
6 tasks
@nytai
Copy link
Member

nytai commented Sep 22, 2020

rerunning with new CI config

@nytai nytai closed this Sep 22, 2020
@nytai nytai reopened this Sep 22, 2020
@ktmud
Copy link
Member

ktmud commented Sep 22, 2020 •
edited
Loading

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

@nytai
Copy link
Member

nytai commented Sep 22, 2020 •
edited
Loading

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.

@graceguo-supercat
Copy link
Author

Thanks for @ktmud quick fix. I will close this PR and favor the new solution!

@mistercrunch
Copy link
Member

+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!

@ktmud
Copy link
Member

ktmud commented Sep 23, 2020

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 should help mitigate this more or less.

ktmud added a commit to ktmud/superset that referenced this pull request Sep 23, 2020
@ktmud
fix: timer component, fixes apache#10849, closes apache#11002
efa13b2
ktmud added a commit that referenced this pull request Sep 23, 2020
@ktmud
fix: timer component, fixes #10849, closes #11002 (#11004)
7549dad
rorymillersoft referenced this pull request in nets-aric/incubator-superset Oct 16, 2020
@rorymillersoft
Merge branch 'master' into superset_additions
320ab1e 
* master: (466 commits)
  chore: bump pandas to latest stable version (#11018)
  fix: dashboard edit button (again) (#11029)
  style(explore): use tertiary button against gray background (#11011)
  docs: add security vulnerability GH issue template (#11023)
  fix: [dashboard] should not show edit button when user has no edit permit (#11024)
  fix: timer component, fixes #10849, closes #11002 (#11004)
  fix: enable several pylint rules partially in db_engines_specs module (#11000)
  fix: pylint checks in connectors/sqla/models.py (#10974)
  fix: reenable pylint rule `unused-import` in charts and connectors modules (#11014)
  Enabled pylint rules in `db_engines` module: (#11016)
  fix: changes a pylint check in dashboard module (#10978)
  fix: menu shows a 0 when there are not settings (#11009)
  fix: query search low privileged user search access denied (#11017)
  chore: downgrade expected exception from error to info (#10994)
  fix: Add Item Overflow on Dataset Editor (#10983)
  Bring back import menu (#11007)
  feat(listview): feature flag config to set default viewing mode (#10986)
  build: add react-hooks linting (#11006)
  fix: unbreak ci (#11003)
  fix: enable pylint rules in db_engine_specs module (#10998)
  ...

# Conflicts:
#	requirements.txt
#	superset/app.py
#	superset/models/schedules.py
#	superset/tasks/schedules.py
#	superset/translations/messages.pot
@graceguo-supercat graceguo-supercat deleted the gg-Revert-10455 branch November 11, 2020 23:17
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@ktmud @auxten
fix: timer component, fixes apache#10849, closes apache#11002 (apache…
1c9cc28 
…#11004)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etr2460 etr2460 etr2460 approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@graceguo-supercat @tanmaylaud @nytai @etr2460 @ktmud @mistercrunch