search

react-monaco-editor / react-monaco-editor

Monaco Editor for React.
MIT License
3.84k stars 378 forks source link

Conversion to use hooks needs more consideration. #660

Open dlech opened 1 year ago

dlech commented 1 year ago

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):

domoritz commented 1 year ago

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

agilgur5 commented 1 year ago

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 commented 1 year ago

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

agilgur5 commented 1 year ago

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.