Skip to content

Commit

Permalink
[React@18] useLayoutEffect when setting value from a prop in `react…
Browse files Browse the repository at this point in the history
…-monaco-editor` (#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.
  • Loading branch information
Dosant authored Oct 17, 2024
1 parent 98ebd09 commit dc3dda7
Show file tree
Hide file tree
Showing 12 changed files with 360 additions and 40 deletions.
26 changes: 26 additions & 0 deletions NOTICE.txt
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,32 @@ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.

---
This code is forked from the `react-monaco-editor`
https://github.com/react-monaco-editor/react-monaco-editor/blob/975cc47b5cb411ee2ffcbdb973daa9342e81a805/src/editor.tsx

The MIT License (MIT)

Copyright (c) 2016-present Leon Shi

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in
all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.

---
This code is part of the Services provided by FullStory, Inc. For license information, please refer to https://www.fullstory.com/legal/terms-and-conditions/
Portions of this code are licensed under the following license:
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1218,7 +1218,6 @@
"react-intl": "6.6.6",
"react-is": "^17.0.2",
"react-markdown": "^6.0.3",
"react-monaco-editor": "^0.54.0",
"react-popper-tooltip": "^3.1.1",
"react-recompose": "^0.33.0",
"react-redux": "^7.2.8",
Expand Down
4 changes: 2 additions & 2 deletions packages/kbn-test/jest-preset.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ module.exports = {

// An array of regexp pattern strings that are matched against all source file paths, matched files will skip transformation
transformIgnorePatterns: [
// ignore all node_modules except monaco-editor, monaco-yaml and react-monaco-editor which requires babel transforms to handle dynamic import()
// ignore all node_modules except monaco-editor, monaco-yaml which requires babel transforms to handle dynamic import()
// since ESM modules are not natively supported in Jest yet (https://github.com/facebook/jest/issues/4842)
'[/\\\\]node_modules(?![\\/\\\\](byte-size|monaco-editor|monaco-yaml|monaco-languageserver-types|monaco-marker-data-provider|monaco-worker-manager|vscode-languageserver-types|react-monaco-editor|d3-interpolate|d3-color|langchain|langsmith|@cfworker|gpt-tokenizer|flat|@langchain))[/\\\\].+\\.js$',
'[/\\\\]node_modules(?![\\/\\\\](byte-size|monaco-editor|monaco-yaml|monaco-languageserver-types|monaco-marker-data-provider|monaco-worker-manager|vscode-languageserver-types|d3-interpolate|d3-color|langchain|langsmith|@cfworker|gpt-tokenizer|flat|@langchain))[/\\\\].+\\.js$',
'packages/kbn-pm/dist/index.js',
'[/\\\\]node_modules(?![\\/\\\\](langchain|langsmith|@langchain))/dist/[/\\\\].+\\.js$',
'[/\\\\]node_modules(?![\\/\\\\](langchain|langsmith|@langchain))/dist/util/[/\\\\].+\\.js$',
Expand Down
1 change: 0 additions & 1 deletion packages/shared-ux/code_editor/impl/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ SRCS = glob(
BUNDLER_DEPS = [
"@npm//react",
"@npm//tslib",
"@npm//react-monaco-editor",
]

js_library(
Expand Down
2 changes: 1 addition & 1 deletion packages/shared-ux/code_editor/impl/README.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ date: 2022-12-05

## Description

This component is an abstraction of the [Monaco Code Editor](https://microsoft.github.io/monaco-editor/) (and the [React Monaco Editor component](https://github.com/react-monaco-editor/react-monaco-editor)). This component still allows access to the other Monaco features.
This component is an abstraction of the [Monaco Code Editor](https://microsoft.github.io/monaco-editor/). This component still allows access to the other Monaco features.

## Usage

Expand Down
6 changes: 2 additions & 4 deletions packages/shared-ux/code_editor/impl/code_editor.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,8 @@ import { MockedMonacoEditor, mockedEditorInstance } from '@kbn/code-editor-mock/

import { CodeEditor } from './code_editor';

jest.mock('react-monaco-editor', () => {
return function JestMockEditor() {
return MockedMonacoEditor;
};
jest.mock('./react_monaco_editor', () => {
return { MonacoEditor: MockedMonacoEditor };
});

// Mock the htmlIdGenerator to generate predictable ids for snapshot tests
Expand Down
28 changes: 5 additions & 23 deletions packages/shared-ux/code_editor/impl/code_editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@
*/

import React, { useState, useRef, useCallback, useMemo, useEffect, KeyboardEvent, FC } from 'react';
import ReactMonacoEditor, {
type MonacoEditorProps as ReactMonacoEditorProps,
} from 'react-monaco-editor';
import {
htmlIdGenerator,
EuiToolTip,
Expand All @@ -34,6 +31,10 @@ import {
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n-react';
import { css } from '@emotion/react';
import {
MonacoEditor as ReactMonacoEditor,
type MonacoEditorProps as ReactMonacoEditorProps,
} from './react_monaco_editor';
import './register_languages';
import { remeasureFonts } from './remeasure_fonts';

Expand Down Expand Up @@ -168,7 +169,7 @@ export interface CodeEditorProps {
export const CodeEditor: React.FC<CodeEditorProps> = ({
languageId,
value,
onChange: _onChange,
onChange,
width,
height,
options,
Expand Down Expand Up @@ -225,8 +226,6 @@ export const CodeEditor: React.FC<CodeEditorProps> = ({

const [isHintActive, setIsHintActive] = useState(true);

const onChange = useBug175684OnChange(_onChange);

const startEditing = useCallback(() => {
setIsHintActive(false);
_editor?.focus();
Expand Down Expand Up @@ -708,23 +707,6 @@ const useFitToContent = ({
}, [editor, isFitToContent, minLines, maxLines, isFullScreen]);
};

// 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']) => {
const onChangePropRef = useRef<CodeEditorProps['onChange']>(onChange);
useEffect(() => {
onChangePropRef.current = onChange;
}, [onChange]);
const onChangeWrapper = useCallback<NonNullable<CodeEditorProps['onChange']>>((_value, event) => {
if (onChangePropRef.current) {
onChangePropRef.current(_value, event);
}
}, []);

return onChangeWrapper;
};

const UseBug177756ReBroadcastMouseDown: FC<{ children: React.ReactNode }> = ({ children }) => {
const [$codeWrapper, setCodeWrapper] = React.useState<HTMLElement | null>(null);

Expand Down
33 changes: 33 additions & 0 deletions packages/shared-ux/code_editor/impl/react_monaco_editor/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
This is a fork of [react-monaco-editor project](https://github.com/react-monaco-editor/react-monaco-editor) that is a Monaco editor wrapper for React.
This fork is needed to apply a change that fixes the editor behavior in Kibana when running React@18 in Legacy Mode and the bug is described [here](https://github.com/facebook/react/issues/31023)
The change is to replace the `useEffect` hook with `useLayoutEffect` when the editor is in controlled mode and the value is updated from prop.

```diff
---useEffect(() => {
+++useLayoutEffect(() => {
if (editor.current) {
if (value === editor.current.getValue()) {
return;
}

const model = editor.current.getModel();
__prevent_trigger_change_event.current = true;
editor.current.pushUndoStop();
// pushEditOperations says it expects a cursorComputer, but doesn't seem to need one.
model.pushEditOperations(
[],
[
{
range: model.getFullModelRange(),
text: value,
},
],
undefined,
);
editor.current.pushUndoStop();
__prevent_trigger_change_event.current = false;
}
}, [value]);
```

In addition, the fork only includes functionality that is used in Kibana and removes the rest of the code that is not needed.
Loading

0 comments on commit dc3dda7

Please sign in to comment.