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.

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

Conversion to use hooks needs more consideration. #660

Open
1 task
dlech opened this issue Nov 29, 2022 · 4 comments
Open
1 task

Conversion to use hooks needs more consideration. #660

dlech opened this issue Nov 29, 2022 · 4 comments

Comments

@dlech
Copy link

dlech commented Nov 29, 2022

Describe the bug
The conversion to hooks in #625 has caused unintentional breaking changes.

The use of useEffect is not a drop-in replacement of componentWillMount, componentDidMount, componentDidUpdate, etc. It is triggered at a different time, so things are in different states which leads to bugs. The editorWillMount, editorDidMount, and editorWillUnmount callbacks probably need to be reconsidered to match the new semantics of useEffect.

To Reproduce
One example is caught by our CI on an automatic depdendabot update.

https://github.com/pybricks/pybricks-code/actions/runs/3576716352/jobs/6014862442

Expected behavior
Breaking changes should come with a major version bump and a migration guide.

Screenshots/Logs
If applicable, add screenshots or logs to help explain your problem.

Environment (please complete the following information):

  • OS: [e.g. Linux]

  • Browser [e.g. Firefox, Safari]

  • Bundler [e.g. webpack]

  • I will try to send a pull request to fix this issue.

@dlech dlech changed the title Conversion to use hooks nees more consideration. Conversion to use hooks needs more consideration. Nov 29, 2022
@domoritz
Copy link
Member

Thanks for the issue. Do you have a few cycles to update the behavior? What's the disadvantage of doing a revert?

@agilgur5
Copy link

agilgur5 commented Aug 28, 2023

Another issue with the refactor is that it broke refs. forwardRef probably needs to be used to match the old behavior. Can see how Argo Workflows uses refs here. CI build failure here.

The lack of changelog made it fairly difficult to find what changed. I eventually did a GitHub compare to just manually check the diffs

@domoritz
Copy link
Member

I'm happy to revert the change. Can you send a pull request?

@agilgur5
Copy link

agilgur5 commented Sep 1, 2023

Mmm I think that's a bit easier said than done too since there have been a few versions on top of the hooks change.

I also don't think the hooks change is a bad thing (it is the new standard, after all), just that there were some breaking changes that were undocumented.

Fixing and documenting any breakage I think would be more of an optimal path forward, IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants