Conversion to use hooks needs more consideration.
Describe the bug The conversion to hooks in https://github.com/react-monaco-editor/react-monaco-editor/pull/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.
Thanks for the issue. Do you have a few cycles to update the behavior? What's the disadvantage of doing a revert?
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
I'm happy to revert the change. Can you send a pull request?
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.