Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -1216,7 +1216,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
2 changes: 1 addition & 1 deletion packages/kbn-test/jest-preset.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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 @@ -701,23 +700,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']) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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