[React@18] useLayoutEffect when setting value from a prop in react-monaco-editor#195775
[React@18] useLayoutEffect when setting value from a prop in react-monaco-editor#195775Dosant merged 5 commits intoelastic:mainfrom
useLayoutEffect when setting value from a prop in react-monaco-editor#195775Conversation
useLayouEffect when setting value from a prop in react-monaco-editor
useLayouEffect when setting value from a prop in react-monaco-editoruseLayoutEffect when setting value from a prop in react-monaco-editor
There was a problem hiding this comment.
this also fixes this bug in the react_monaco_editor, so this workaround in the parent component is no longer needed
There was a problem hiding this comment.
the main change was to change from useEffect -> useLayoutEffect here
There was a problem hiding this comment.
this is a bummer, but I think it is an OK tradeoff as this seem to be private and doesn't affect consumers. Otherwise this would require a larger restructure and another package for react-monaco-editor folder and there is no need for that
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
There was a problem hiding this comment.
Can we follow the @notice pattern, example in https://github.com/elastic/kibana/blob/main/kbn_pm/src/lib/normalize_path.mjs#L3-L28
After we'll have to regenerate the notice file with node scripts/notice
There was a problem hiding this comment.
Thanks, so I understand with this approach I can simplify and don't do a separate header with linting and license file? will do, thanks
|
I am quite positive with the CI been green but I am going to do a thorough test too Anton. |
stratoula
left a comment
There was a problem hiding this comment.
ES|QL editor looks good Anton
90b9ff7 to
8a4f481
Compare
packages/kbn-test/jest-preset.js
Outdated
There was a problem hiding this comment.
We should also update this comment to remove the reference to react-monaco-editor
|
@elasticmachine merge upstream |
pheyos
left a comment
There was a problem hiding this comment.
packages/kbn-test/jest-preset.js changes LGTM
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
|
|
Starting backport for target branches: 8.x |
…-monaco-editor` (elastic#195775) ## Summary This PR is part of elastic/kibana-team#1016 (comment) and needed to upgrade Kibana to React@18 in Legacy mode. We've found a problem in `react-monaco-editor` component which is a very thin wrapper around `monaco` where it doesn't play nicely with React@18 in legacy mode. You can read on details around the issue here elastic/eui#8018 where we've found a similar issue in EUI's search bar component. The workaround we've found is to change `useEffect` -> `useLayouEffect` where the value that is coming from the prop is set into the model. The issue and a fix might be a bit controversal and the component is very thin, so I decided that it might be better to bring the fork into Kibana with only what's needed and with a workaround. (cherry picked from commit dc3dda7)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…a prop in `react-monaco-editor` (#195775) (#196671) # Backport This will backport the following commits from `main` to `8.x`: - [[React@18] `useLayoutEffect` when setting value from a prop in `react-monaco-editor` (#195775)](#195775) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Anton Dosov","email":"anton.dosov@elastic.co"},"sourceCommit":{"committedDate":"2024-10-17T11:24:06Z","message":"[React@18] `useLayoutEffect` when setting value from a prop in `react-monaco-editor` (#195775)\n\n## Summary\r\n\r\nThis PR is part of\r\nhttps://github.com/elastic/kibana-team/issues/1016#issuecomment-2399310175\r\nand needed to upgrade Kibana to React@18 in Legacy mode.\r\n\r\nWe've found a problem in `react-monaco-editor` component which is a very\r\nthin wrapper around `monaco` where it doesn't play nicely with React@18\r\nin legacy mode. You can read on details around the issue here\r\nhttps://github.com/elastic/eui/issues/8018 where we've found a similar\r\nissue in EUI's search bar component. The workaround we've found is to\r\nchange `useEffect` -> `useLayouEffect` where the value that is coming\r\nfrom the prop is set into the model. The issue and a fix might be a bit\r\ncontroversal and the component is very thin, so I decided that it might\r\nbe better to bring the fork into Kibana with only what's needed and with\r\na workaround.","sha":"dc3dda7d12662f3d7b5cb6c6c6366e07eae138fa","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:SharedUX","backport:prev-minor"],"title":"[React@18] `useLayoutEffect` when setting value from a prop in `react-monaco-editor`","number":195775,"url":"https://github.com/elastic/kibana/pull/195775","mergeCommit":{"message":"[React@18] `useLayoutEffect` when setting value from a prop in `react-monaco-editor` (#195775)\n\n## Summary\r\n\r\nThis PR is part of\r\nhttps://github.com/elastic/kibana-team/issues/1016#issuecomment-2399310175\r\nand needed to upgrade Kibana to React@18 in Legacy mode.\r\n\r\nWe've found a problem in `react-monaco-editor` component which is a very\r\nthin wrapper around `monaco` where it doesn't play nicely with React@18\r\nin legacy mode. You can read on details around the issue here\r\nhttps://github.com/elastic/eui/issues/8018 where we've found a similar\r\nissue in EUI's search bar component. The workaround we've found is to\r\nchange `useEffect` -> `useLayouEffect` where the value that is coming\r\nfrom the prop is set into the model. The issue and a fix might be a bit\r\ncontroversal and the component is very thin, so I decided that it might\r\nbe better to bring the fork into Kibana with only what's needed and with\r\na workaround.","sha":"dc3dda7d12662f3d7b5cb6c6c6366e07eae138fa"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195775","number":195775,"mergeCommit":{"message":"[React@18] `useLayoutEffect` when setting value from a prop in `react-monaco-editor` (#195775)\n\n## Summary\r\n\r\nThis PR is part of\r\nhttps://github.com/elastic/kibana-team/issues/1016#issuecomment-2399310175\r\nand needed to upgrade Kibana to React@18 in Legacy mode.\r\n\r\nWe've found a problem in `react-monaco-editor` component which is a very\r\nthin wrapper around `monaco` where it doesn't play nicely with React@18\r\nin legacy mode. You can read on details around the issue here\r\nhttps://github.com/elastic/eui/issues/8018 where we've found a similar\r\nissue in EUI's search bar component. The workaround we've found is to\r\nchange `useEffect` -> `useLayouEffect` where the value that is coming\r\nfrom the prop is set into the model. The issue and a fix might be a bit\r\ncontroversal and the component is very thin, so I decided that it might\r\nbe better to bring the fork into Kibana with only what's needed and with\r\na workaround.","sha":"dc3dda7d12662f3d7b5cb6c6c6366e07eae138fa"}}]}] BACKPORT--> Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
Summary
This PR is part of https://github.com/elastic/kibana-team/issues/1016#issuecomment-2399310175 and needed to upgrade Kibana to React@18 in Legacy mode.
We've found a problem in
react-monaco-editorcomponent which is a very thin wrapper aroundmonacowhere it doesn't play nicely with React@18 in legacy mode. You can read on details around the issue here elastic/eui#8018 where we've found a similar issue in EUI's search bar component. The workaround we've found is to changeuseEffect->useLayouEffectwhere the value that is coming from the prop is set into the model. The issue and a fix might be a bit controversal and the component is very thin, so I decided that it might be better to bring the fork into Kibana with only what's needed and with a workaround.(fyi @elastic/kibana-management @elastic/kibana-esql as the largest users of
CodeEditor)