[React@18] `useLayoutEffect` when setting value from a prop in `react-monaco-editor` #195775

Dosant · 2024-10-10T13:56:22Z

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

(fyi @elastic/kibana-management @elastic/kibana-esql as the largest users of CodeEditor)

.eslintrc.js

Dosant · 2024-10-14T10:22:12Z

packages/shared-ux/code_editor/impl/code_editor.tsx

-// https://github.com/elastic/kibana/issues/175684
-// 'react-monaco-editor' has a bug that it always calls the initial onChange callback, so the closure might become stale
-// we work this around by calling the latest onChange from props
-const useBug175684OnChange = (onChange: CodeEditorProps['onChange']) => {


this also fixes this bug in the react_monaco_editor, so this workaround in the parent component is no longer needed

Dosant · 2024-10-14T10:22:35Z

packages/shared-ux/code_editor/impl/react_monaco_editor/editor.tsx

+  useEffect(initMonaco, []);
+
+  // useLayoutEffect instead of useEffect to mitigate https://github.com/facebook/react/issues/31023 in React@18 Legacy Mode
+  useLayoutEffect(() => {


the main change was to change from useEffect -> useLayoutEffect here

Dosant · 2024-10-14T10:24:08Z

packages/shared-ux/code_editor/mocks/monaco_mock/index.tsx

 import { monaco } from '@kbn/monaco';
+// TODO: circular dependency
+// import type { MonacoEditorProps } from '@kbn/code-editor/react_monaco_editor';
+type MonacoEditorProps = any;


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

elasticmachine · 2024-10-14T10:31:48Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

jbudz · 2024-10-14T13:48:50Z

packages/shared-ux/code_editor/impl/react_monaco_editor/types.ts

@@ -0,0 +1,93 @@
+/*


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

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

stratoula · 2024-10-15T07:02:39Z

I am quite positive with the CI been green but I am going to do a thorough test too Anton.

stratoula

ES|QL editor looks good Anton

eokoneyo

Code changes LGTM

eokoneyo · 2024-10-14T22:14:21Z

packages/kbn-test/jest-preset.js

@@ -107,7 +107,7 @@ module.exports = {
  transformIgnorePatterns: [
    // ignore all node_modules except monaco-editor, monaco-yaml and react-monaco-editor which requires babel transforms to handle dynamic import()


We should also update this comment to remove the reference to react-monaco-editor

Dosant · 2024-10-16T09:34:41Z

@elasticmachine merge upstream

pheyos

packages/kbn-test/jest-preset.js changes LGTM

Dosant · 2024-10-17T09:27:03Z

@elasticmachine merge upstream

elasticmachine · 2024-10-17T11:14:56Z

💚 Build Succeeded

Buildkite Build
Commit: 08dd4cc

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`@kbn/code-editor`	0	1	+1

ESLint disabled line counts

id	before	after	diff
`@kbn/code-editor`	3	5	+2

Total ESLint disabled count

id	before	after	diff
`@kbn/code-editor`	3	6	+3

History

💔 Build #243003 failed d2511bd
💛 Build #242571 was flaky 6cc172e
💚 Build #242476 succeeded 8a4f481
💚 Build #242150 succeeded 90b9ff782b25d979de2572bf940f7c3965d49eb5
💛 Build #241459 was flaky 0bb389368bfd8f0b7d1e69546157a4af286d45b7

kibanamachine · 2024-10-17T11:24:25Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11383942184

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

kibanamachine · 2024-10-17T11:29:02Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

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)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Anton Dosov <[email protected]>

Dosant changed the title ~~codeeditor fix~~ [React@18] useLayouEffect when setting value from a prop in react-monaco-editor Oct 14, 2024

Dosant changed the title ~~[React@18] useLayouEffect when setting value from a prop in react-monaco-editor~~ [React@18] useLayoutEffect when setting value from a prop in react-monaco-editor Oct 14, 2024

Dosant commented Oct 14, 2024

View reviewed changes

Dosant added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 14, 2024

Dosant marked this pull request as ready for review October 14, 2024 10:31

Dosant requested review from a team as code owners October 14, 2024 10:31

Dosant requested a review from eokoneyo October 14, 2024 11:32

jbudz reviewed Oct 14, 2024

View reviewed changes

stratoula approved these changes Oct 15, 2024

View reviewed changes

simplify license setup

8a4f481

Dosant force-pushed the react18/codeeditor branch from 90b9ff7 to 8a4f481 Compare October 15, 2024 09:08

Dosant requested a review from jbudz October 15, 2024 09:09

eokoneyo approved these changes Oct 15, 2024

View reviewed changes

Dosant added 2 commits October 15, 2024 13:59

Merge branch 'main' of github.com:elastic/kibana into react18/codeeditor

87cefca

clean comment

6cc172e

jbudz approved these changes Oct 15, 2024

View reviewed changes

Merge branch 'main' into react18/codeeditor

d2511bd

pheyos approved these changes Oct 16, 2024

View reviewed changes

Merge branch 'main' into react18/codeeditor

08dd4cc

Dosant merged commit dc3dda7 into elastic:main Oct 17, 2024
38 checks passed

kibanamachine added the v9.0.0 label Oct 17, 2024

kibanamachine mentioned this pull request Oct 17, 2024

[8.x] [React@18] `useLayoutEffect` when setting value from a prop in `react-monaco-editor` (#195775) #196671

Merged

kibanamachine mentioned this pull request Oct 17, 2024

[ci] Extract OAS check + add retry #196534

Merged

kibanamachine added the v8.17.0 label Oct 17, 2024

Dosant added the React@18 label Nov 6, 2024

Dosant mentioned this pull request Dec 5, 2024

[Meta] React@18 legacy mode upgrade QA #203114

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[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` #195775

Dosant commented Oct 10, 2024 •

edited by kibanamachine

Loading

Dosant Oct 14, 2024

Dosant Oct 14, 2024

Dosant Oct 14, 2024

elasticmachine commented Oct 14, 2024

jbudz Oct 14, 2024

Dosant Oct 15, 2024

stratoula commented Oct 15, 2024

stratoula left a comment

eokoneyo left a comment

eokoneyo Oct 14, 2024

Dosant commented Oct 16, 2024

pheyos left a comment

Dosant commented Oct 17, 2024

elasticmachine commented Oct 17, 2024

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Oct 17, 2024

kibanamachine commented Oct 17, 2024

		@@ -107,7 +107,7 @@ module.exports = {
		transformIgnorePatterns: [
		// ignore all node_modules except monaco-editor, monaco-yaml and react-monaco-editor which requires babel transforms to handle dynamic import()

[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 #195775

Conversation

Dosant commented Oct 10, 2024 • edited by kibanamachine Loading

Summary

Dosant Oct 14, 2024

Choose a reason for hiding this comment

Dosant Oct 14, 2024

Choose a reason for hiding this comment

Dosant Oct 14, 2024

Choose a reason for hiding this comment

elasticmachine commented Oct 14, 2024

jbudz Oct 14, 2024

Choose a reason for hiding this comment

Dosant Oct 15, 2024

Choose a reason for hiding this comment

stratoula commented Oct 15, 2024

stratoula left a comment

Choose a reason for hiding this comment

eokoneyo left a comment

Choose a reason for hiding this comment

eokoneyo Oct 14, 2024

Choose a reason for hiding this comment

Dosant commented Oct 16, 2024

pheyos left a comment

Choose a reason for hiding this comment

Dosant commented Oct 17, 2024

elasticmachine commented Oct 17, 2024

💚 Build Succeeded

Metrics [docs]

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Oct 17, 2024

kibanamachine commented Oct 17, 2024

💚 All backports created successfully

Questions ?

[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` #195775

Dosant commented Oct 10, 2024 •

edited by kibanamachine

Loading