From 1120c2b235d2ca2c8b14c818ccfc2847294c3811 Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Wed, 16 Aug 2023 16:35:59 -0500 Subject: [PATCH] fix: Upgrade Monaco to ^0.41.0 (#1448) Fixes #1445, fixes #1191 Fixing Monaco bug by upgrading to latest ^0.41.0 Supports DH-15438 BREAKING CHANGE: Monaco will need to be upgraded to ^0.41.0 in Enterprise to ensure compatibility **Tests Performed** - Console Input - `Cmd+F` does nothing - Intellisense can be closed via `Esc` - Log tab - `Esc` does not close find input - `Esc` does clear selection when focus is in the log content - Code Editor - Verified that newline with leading space no longer crashes the browser tab ``` a a ``` - Wrote some Python code. Intellisense, syntax highlighting, and general typing experience seemed as expected - Execute full code + selected code successfully --- README.md | 2 +- jest.setup.ts | 22 ++++ package-lock.json | 31 ++--- package.json | 1 + packages/code-studio/package.json | 2 +- packages/console/package.json | 5 +- packages/console/src/ConsoleInput.tsx | 10 +- packages/console/src/log/LogView.tsx | 55 ++++----- .../console/src/monaco/MonacoUtils.test.ts | 55 +++++++++ packages/console/src/monaco/MonacoUtils.ts | 114 +++++++----------- packages/console/src/notebook/Editor.tsx | 3 +- packages/iris-grid/package.json | 2 +- .../src/sidebar/CustomColumnBuilder.test.tsx | 5 +- 13 files changed, 178 insertions(+), 129 deletions(-) diff --git a/README.md b/README.md index f7e35a8c2f..28e3c67061 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ We use [Playwright](https://playwright.dev/) for end-to-end tests. We test again You should be able to pass arguments to these commands as if you were running Playwright via CLI directly. For example, to test only `table.spec.ts` you could run `npm run e2e -- ./tests/table.spec.ts`, or to test only `table.spec.ts` in Firefox, you could run `npm run e2e -- --project firefox ./tests/table.spec.ts`. See [Playwright CLI](https://playwright.dev/docs/test-cli) for more details. -Snapshots are used by end-to-end tests to visually verify the output. Snapshots are both OS and broser specific. Sometimes changes are made requiring snapshots to be updated. Update snapshots locally by running `npm run e2e:update-snapshots`. +Snapshots are used by end-to-end tests to visually verify the output. Snapshots are both OS and browser specific. Sometimes changes are made requiring snapshots to be updated. Update snapshots locally by running `npm run e2e:update-snapshots`. Once you are satisfied with the snapshots and everything is passing locally, you need to use the docker image to update snapshots for CI (unless you are running the same platform as CI (Ubuntu)). Run `npm run e2e:update-ci-snapshots` to update the CI snapshots. The snapshots will be written to your local directories. The Linux snapshots should be committed to git (non-Linux snapshots should be automatically ignored by git). diff --git a/jest.setup.ts b/jest.setup.ts index c38b2c3cda..cd53ccf1fe 100644 --- a/jest.setup.ts +++ b/jest.setup.ts @@ -1,7 +1,10 @@ import '@testing-library/jest-dom'; +import { TextDecoder, TextEncoder } from 'util'; +import { performance } from 'perf_hooks'; import 'jest-canvas-mock'; import './__mocks__/dh-core'; import Log from '@deephaven/log'; +import { TestUtils } from '@deephaven/utils'; let logLevel = parseInt(process.env.DH_LOG_LEVEL ?? '', 10); if (!Number.isFinite(logLevel)) { @@ -30,6 +33,25 @@ Object.defineProperty(window, 'matchMedia', { })), }); +Object.defineProperty(window, 'performance', { + value: performance, + writable: true, +}); + +Object.defineProperty(window, 'ResizeObserver', { + value: function () { + return TestUtils.createMockProxy(); + }, +}); + +Object.defineProperty(window, 'TextDecoder', { + value: TextDecoder, +}); + +Object.defineProperty(window, 'TextEncoder', { + value: TextEncoder, +}); + Object.defineProperty(document, 'fonts', { value: { ready: Promise.resolve(), diff --git a/package-lock.json b/package-lock.json index 5da96cc4d1..8f1afb1f82 100644 --- a/package-lock.json +++ b/package-lock.json @@ -57,6 +57,7 @@ "@deephaven/redux": "file:../redux", "@deephaven/stylelint-config": "file:../stylelint-config", "@deephaven/tsconfig": "file:../tsconfig", + "@deephaven/utils": "file:../utils", "@fortawesome/fontawesome-common-types": "^6.1.1", "@playwright/test": "^1.30.0", "@testing-library/jest-dom": "^5.16.4", @@ -18621,9 +18622,9 @@ } }, "node_modules/monaco-editor": { - "version": "0.31.1", - "resolved": "https://registry.npmjs.org/monaco-editor/-/monaco-editor-0.31.1.tgz", - "integrity": "sha512-FYPwxGZAeP6mRRyrr5XTGHD9gRXVjy7GUzF4IPChnyt3fS5WrNxIkS8DNujWf6EQy0Zlzpxw8oTVE+mWI2/D1Q==" + "version": "0.41.0", + "resolved": "https://registry.npmjs.org/monaco-editor/-/monaco-editor-0.41.0.tgz", + "integrity": "sha512-1o4olnZJsiLmv5pwLEAmzHTE/5geLKQ07BrGxlF4Ri/AXAc2yyDGZwHjiTqD8D/ROKUZmwMA28A+yEowLNOEcA==" }, "node_modules/moo-color": { "version": "1.0.3", @@ -25237,7 +25238,7 @@ "lodash.throttle": "^4.1.1", "memoize-one": "^5.1.1", "memoizee": "^0.4.15", - "monaco-editor": "^0.31.1", + "monaco-editor": "^0.41.0", "pouchdb-browser": "^7.2.2", "pouchdb-find": "^7.2.2", "prop-types": "^15.7.2", @@ -25349,11 +25350,12 @@ "lodash.throttle": "^4.1.1", "memoize-one": "^5.1.1", "memoizee": "^0.4.15", - "monaco-editor": "^0.31.1", + "monaco-editor": "^0.41.0", "papaparse": "5.3.2", "popper.js": "^1.16.1", "prop-types": "^15.7.2", - "shell-quote": "^1.7.2" + "shell-quote": "^1.7.2", + "shortid": "^2.2.16" }, "devDependencies": { "@deephaven/jsapi-shim": "file:../jsapi-shim", @@ -25646,7 +25648,7 @@ "lodash.throttle": "^4.1.1", "memoize-one": "^5.1.1", "memoizee": "^0.4.15", - "monaco-editor": "^0.31.1", + "monaco-editor": "^0.41.0", "prop-types": "^15.7.2", "react-beautiful-dnd": "^13.1.0", "react-transition-group": "^4.4.2", @@ -27188,7 +27190,7 @@ "lodash.throttle": "^4.1.1", "memoize-one": "^5.1.1", "memoizee": "^0.4.15", - "monaco-editor": "^0.31.1", + "monaco-editor": "^0.41.0", "pouchdb-browser": "^7.2.2", "pouchdb-find": "^7.2.2", "prop-types": "^15.7.2", @@ -27264,11 +27266,12 @@ "lodash.throttle": "^4.1.1", "memoize-one": "^5.1.1", "memoizee": "^0.4.15", - "monaco-editor": "^0.31.1", + "monaco-editor": "^0.41.0", "papaparse": "5.3.2", "popper.js": "^1.16.1", "prop-types": "^15.7.2", - "shell-quote": "^1.7.2" + "shell-quote": "^1.7.2", + "shortid": "^2.2.16" } }, "@deephaven/dashboard": { @@ -27468,7 +27471,7 @@ "lodash.throttle": "^4.1.1", "memoize-one": "^5.1.1", "memoizee": "^0.4.15", - "monaco-editor": "^0.31.1", + "monaco-editor": "^0.41.0", "prop-types": "^15.7.2", "react-beautiful-dnd": "^13.1.0", "react-transition-group": "^4.4.2", @@ -39343,9 +39346,9 @@ } }, "monaco-editor": { - "version": "0.31.1", - "resolved": "https://registry.npmjs.org/monaco-editor/-/monaco-editor-0.31.1.tgz", - "integrity": "sha512-FYPwxGZAeP6mRRyrr5XTGHD9gRXVjy7GUzF4IPChnyt3fS5WrNxIkS8DNujWf6EQy0Zlzpxw8oTVE+mWI2/D1Q==" + "version": "0.41.0", + "resolved": "https://registry.npmjs.org/monaco-editor/-/monaco-editor-0.41.0.tgz", + "integrity": "sha512-1o4olnZJsiLmv5pwLEAmzHTE/5geLKQ07BrGxlF4Ri/AXAc2yyDGZwHjiTqD8D/ROKUZmwMA28A+yEowLNOEcA==" }, "moo-color": { "version": "1.0.3", diff --git a/package.json b/package.json index e29e4a1a00..e614f8ebe0 100644 --- a/package.json +++ b/package.json @@ -69,6 +69,7 @@ "@deephaven/redux": "file:../redux", "@deephaven/stylelint-config": "file:../stylelint-config", "@deephaven/tsconfig": "file:../tsconfig", + "@deephaven/utils": "file:../utils", "@fortawesome/fontawesome-common-types": "^6.1.1", "@playwright/test": "^1.30.0", "@testing-library/jest-dom": "^5.16.4", diff --git a/packages/code-studio/package.json b/packages/code-studio/package.json index a2e936857b..93223936b8 100644 --- a/packages/code-studio/package.json +++ b/packages/code-studio/package.json @@ -44,7 +44,7 @@ "lodash.throttle": "^4.1.1", "memoize-one": "^5.1.1", "memoizee": "^0.4.15", - "monaco-editor": "^0.31.1", + "monaco-editor": "^0.41.0", "pouchdb-browser": "^7.2.2", "pouchdb-find": "^7.2.2", "prop-types": "^15.7.2", diff --git a/packages/console/package.json b/packages/console/package.json index a3b97b1773..1d690f393d 100644 --- a/packages/console/package.json +++ b/packages/console/package.json @@ -38,11 +38,12 @@ "lodash.throttle": "^4.1.1", "memoize-one": "^5.1.1", "memoizee": "^0.4.15", - "monaco-editor": "^0.31.1", + "monaco-editor": "^0.41.0", "papaparse": "5.3.2", "popper.js": "^1.16.1", "prop-types": "^15.7.2", - "shell-quote": "^1.7.2" + "shell-quote": "^1.7.2", + "shortid": "^2.2.16" }, "peerDependencies": { "react": "^17.x", diff --git a/packages/console/src/ConsoleInput.tsx b/packages/console/src/ConsoleInput.tsx index e6be8fba09..552dd44492 100644 --- a/packages/console/src/ConsoleInput.tsx +++ b/packages/console/src/ConsoleInput.tsx @@ -187,6 +187,7 @@ export class ConsoleInput extends PureComponent< const element = this.commandContainer.current; assertNotNull(element); + this.commandEditor = monaco.editor.create(element, commandSettings); MonacoUtils.setEOL(this.commandEditor); @@ -283,13 +284,10 @@ export class ConsoleInput extends PureComponent< } }); - // Override the Ctrl+F functionality so that the find window doesn't appear - this.commandEditor.addCommand( + // Disable the Ctrl+F functionality so that the find window doesn't appear + MonacoUtils.disableKeyBindings(this.commandEditor, [ monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyF, // eslint-disable-line no-bitwise - () => undefined - ); - - MonacoUtils.removeConflictingKeybindings(this.commandEditor); + ]); MonacoUtils.registerPasteHandler(this.commandEditor); diff --git a/packages/console/src/log/LogView.tsx b/packages/console/src/log/LogView.tsx index dba2fef1c3..ed9be7e342 100644 --- a/packages/console/src/log/LogView.tsx +++ b/packages/console/src/log/LogView.tsx @@ -9,6 +9,7 @@ import ConsoleUtils from '../common/ConsoleUtils'; import LogLevel from './LogLevel'; import './LogView.scss'; import LogLevelMenuItem from './LogLevelMenuItem'; +import { MonacoUtils } from '../monaco'; interface LogViewProps { session: IdeSession; @@ -226,36 +227,30 @@ class LogView extends PureComponent { wordWrap: 'on', }); - // When find widget is open, escape key closes it. - // Instead, capture it and do nothing. Same for shift-escape. - this.editor.addCommand(monaco.KeyCode.Escape, () => undefined); - this.editor.addCommand( - // eslint-disable-next-line no-bitwise - monaco.KeyMod.Shift | monaco.KeyCode.Escape, - () => undefined - ); - - // Restore regular escape to clear selection, when editorText has focus. - this.editor.addCommand( - monaco.KeyCode.Escape, - () => { - const position = this.editor?.getPosition(); - assertNotNull(position); - this.editor?.setPosition(position); - }, - 'findWidgetVisible && editorTextFocus' - ); - - this.editor.addCommand( - // eslint-disable-next-line no-bitwise - monaco.KeyMod.Shift | monaco.KeyCode.Escape, - () => { - const position = this.editor?.getPosition(); - assertNotNull(position); - this.editor?.setPosition(position); - }, - 'findWidgetVisible && editorTextFocus' - ); + // Override default Monaco keybindings for `escape` and `shift-escape` + [ + [monaco.KeyCode.Escape], + [monaco.KeyMod.Shift | monaco.KeyCode.Escape], // eslint-disable-line no-bitwise + ].forEach(keybindings => { + assertNotNull(this.editor); + + // Monaco default behavior is for escape key to close the find widget. + // Instead, capture it and do nothing. Same for shift-escape. + MonacoUtils.disableKeyBindings(this.editor, keybindings); + + // Restore regular escape to clear selection, when editorText has focus. + this.editor.addAction({ + id: 'clear-selection-on-escape', + label: '', + keybindings: [monaco.KeyCode.Escape], + keybindingContext: 'findWidgetVisible && editorTextFocus', + run: () => { + const position = this.editor?.getPosition(); + assertNotNull(position); + this.editor?.setPosition(position); + }, + }); + }); } destroyMonaco(): void { diff --git a/packages/console/src/monaco/MonacoUtils.test.ts b/packages/console/src/monaco/MonacoUtils.test.ts index 702bba1d41..85cdea53ba 100644 --- a/packages/console/src/monaco/MonacoUtils.test.ts +++ b/packages/console/src/monaco/MonacoUtils.test.ts @@ -1,8 +1,11 @@ /* eslint-disable no-bitwise */ import * as monaco from 'monaco-editor'; import { Shortcut, KEY, MODIFIER } from '@deephaven/components'; +import { TestUtils } from '@deephaven/utils'; import MonacoUtils from './MonacoUtils'; +const { asMock, createMockProxy } = TestUtils; + const SINGLE_KEY_PARAMS: ConstructorParameters[0] = { id: 'Single key', name: '', @@ -45,6 +48,11 @@ const MULTI_MOD_PARAMS: ConstructorParameters[0] = { macShortcut: [MODIFIER.CMD, MODIFIER.SHIFT, KEY.B], }; +beforeEach(() => { + jest.clearAllMocks(); + expect.hasAssertions(); +}); + describe('Register worker', () => { it('Registers the getWorker function', () => { const getWorker = () => ({}) as Worker; @@ -151,6 +159,23 @@ describe('Mac shortcuts', () => { }); }); +describe('disableKeyBindings', () => { + const editor = createMockProxy(); + + it('should disable key bindings for the given editor', () => { + const keybindings = [1, 2, 3]; + + MonacoUtils.disableKeyBindings(editor, keybindings); + + expect(editor.addAction).toHaveBeenCalledWith({ + id: expect.stringMatching(/^disable-keybindings-.+/), + label: '', + keybindings, + run: expect.any(Function), + }); + }); +}); + describe('provideLinks', () => { it('it should get a provideLinks function which should return an object with the links', () => { const { provideLinks } = MonacoUtils; @@ -177,3 +202,33 @@ describe('provideLinks', () => { expect(provideLinks(mockModel)).toEqual(expectedValue); }); }); + +describe('removeConflictingKeybindings', () => { + beforeEach(() => { + jest.spyOn(monaco.editor, 'addKeybindingRule'); + jest.spyOn(MonacoUtils, 'isMacPlatform'); + }); + + it.each([true, false])( + 'should override keybinding rules - isMac:%s', + isMac => { + asMock(MonacoUtils.isMacPlatform).mockReturnValue(isMac); + + MonacoUtils.removeConflictingKeybindings(); + + expect(monaco.editor.addKeybindingRule).toHaveBeenCalledWith({ + keybinding: monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyD, + command: null, + }); + + if (isMac) { + expect(monaco.editor.addKeybindingRule).toHaveBeenCalledTimes(1); + } else { + expect(monaco.editor.addKeybindingRule).toHaveBeenCalledWith({ + keybinding: monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyH, + command: null, + }); + } + } + ); +}); diff --git a/packages/console/src/monaco/MonacoUtils.ts b/packages/console/src/monaco/MonacoUtils.ts index 50f26f8e5a..26a9fb54ee 100644 --- a/packages/console/src/monaco/MonacoUtils.ts +++ b/packages/console/src/monaco/MonacoUtils.ts @@ -1,4 +1,5 @@ /* eslint-disable @typescript-eslint/ban-ts-comment */ +import shortid from 'shortid'; /** * Exports a function for initializing monaco with the deephaven theme/config */ @@ -158,6 +159,8 @@ class MonacoUtils { registerLanguages([DbLang, PyLang, GroovyLang, LogLang, ScalaLang]); + MonacoUtils.removeConflictingKeybindings(); + log.debug('Monaco initialized.'); } @@ -341,89 +344,56 @@ class MonacoUtils { /** * Remove any keybindings which are used for our own shortcuts. - * This allows the key events to bubble up so a component higher up can capture them - * @param editor The editor to remove the keybindings from + * This allows the key events to bubble up so a component higher up can capture + * them. Note that this is a global configuration, so all editor instances will + * be impacted. */ - static removeConflictingKeybindings( - editor: monaco.editor.IStandaloneCodeEditor - ): void { - // Multi-mod key events have a specific order - // E.g. ctrl+alt+UpArrow is not found, but alt+ctrl+UpArrow is found - // meta is WindowsKey on Windows and cmd on Mac - // ctrl is ctrl Windows and ctrl on Mac - // alt is alt on Windows and option on Mac - const keybindings = [ - { - windows: 'ctrl+D', - mac: 'meta+D', - }, + static removeConflictingKeybindings(): void { + // All editor instances share a global keybinding registry which is where + // default keybindings are set. There doesn't appear to be a way to remove + // default bindings, but we can add new ones that will override the existing + // ones and set `command` to null. This will treat the key events as unhandled + // and allow them to bubble up. + monaco.editor.addKeybindingRule( + // Restart console in Enterprise (see Shortcuts.ts) { - windows: 'ctrl+H', - }, - ]; + /* eslint-disable-next-line no-bitwise */ + keybinding: monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyD, + command: null, + } + ); - try { - keybindings.forEach(keybinding => { - if ( - (MonacoUtils.isMacPlatform() && keybinding.mac === '') || - (!MonacoUtils.isMacPlatform() && keybinding.windows === '') - ) { - return; - } - MonacoUtils.removeKeybinding( - editor, - MonacoUtils.isMacPlatform() ? keybinding.mac : keybinding.windows - ); + // Ctrl+H is used to focus Community console history in Windows + Linux. + // An alternate shortcut is used for Mac, so no need to override it + // (See ConsoleShortcuts.ts) + if (!MonacoUtils.isMacPlatform()) { + monaco.editor.addKeybindingRule({ + /* eslint-disable-next-line no-bitwise */ + keybinding: monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyH, + command: null, }); - } catch (err) { - // This is probably only caused by Monaco changing private methods used here - log.error(err); } } /** - * Remove a keybinding and allow the events to bubble up - * If you want the keybinding removed for all editors, add it to removeConflictingKeybindings - * Monaco still captures the event if you choose to override the keybinding with a no-op function - * - * Based on the following comment to remove a keybinding and let the event bubble up - * https://github.com/microsoft/monaco-editor/issues/287#issuecomment-331447475 - * The issue for an API for this has apparently been open since 2016. Link below - * https://github.com/microsoft/monaco-editor/issues/102 - * @param editor The editor to remove the keybinding from - * @param keybinding The key string to remove. E.g. 'ctrl+C' for copy on Windows + * Creates actions with a `noop` run function to disable specified keybindings. + * Note that this will swallow the events. To disable default keybindings in a + * way that allows events to propagate upward, see `removeConflictingKeybindings` + * @param editor Editor to disable keybindings for + * @param keybindings List of monaco keybindings to disable. Often a bitwise + * combination like `monaco.KeyMod.Alt | monaco.KeyMod.KeyJ` */ - static removeKeybinding( + static disableKeyBindings( editor: monaco.editor.IStandaloneCodeEditor, - keybinding: string | undefined + keybindings: number[] ): void { - if (keybinding === undefined) { - return; - } - - /* eslint-disable no-underscore-dangle */ - // It's possible a single keybinding has multiple commands depending on context - // @ts-ignore - const keybindings = editor._standaloneKeybindingService - ._getResolver() - ._map.get(keybinding); - - if (keybindings != null) { - keybindings.forEach((elem: { command: unknown }) => { - log.debug2( - `Removing Monaco keybinding ${keybinding} for ${elem.command}` - ); - // @ts-ignore - editor._standaloneKeybindingService.addDynamicKeybinding( - `-${elem.command}`, - null, - () => undefined - ); - }); - } else { - log.warn(`Did not find any keybindings to remove for ${keybinding}`); - } - /* eslint-enable no-underscore-dangle */ + editor.addAction({ + // This shouldn't be referenced by anything so using an arbitrary unique id + id: `disable-keybindings-${shortid()}`, + label: '', // This action won't be shown in the UI so no need for a label + keybindings, + run: () => undefined, + }); } static getMonacoKeyCodeFromShortcut(shortcut: Shortcut): number { diff --git a/packages/console/src/notebook/Editor.tsx b/packages/console/src/notebook/Editor.tsx index 3ec723a321..2774cc70b1 100644 --- a/packages/console/src/notebook/Editor.tsx +++ b/packages/console/src/notebook/Editor.tsx @@ -94,7 +94,9 @@ class Editor extends Component> { ...settings, }; assertNotNull(this.container); + this.editor = monaco.editor.create(this.container, settings); + this.editor.addAction({ id: 'find', label: 'Find', @@ -112,7 +114,6 @@ class Editor extends Component> { }, }); this.editor.layout(); - MonacoUtils.removeConflictingKeybindings(this.editor); monaco.languages.registerLinkProvider('plaintext', { provideLinks: MonacoUtils.provideLinks, diff --git a/packages/iris-grid/package.json b/packages/iris-grid/package.json index d12a206a11..fc6be2c0ab 100644 --- a/packages/iris-grid/package.json +++ b/packages/iris-grid/package.json @@ -45,7 +45,7 @@ "lodash.throttle": "^4.1.1", "memoize-one": "^5.1.1", "memoizee": "^0.4.15", - "monaco-editor": "^0.31.1", + "monaco-editor": "^0.41.0", "prop-types": "^15.7.2", "react-beautiful-dnd": "^13.1.0", "react-transition-group": "^4.4.2", diff --git a/packages/iris-grid/src/sidebar/CustomColumnBuilder.test.tsx b/packages/iris-grid/src/sidebar/CustomColumnBuilder.test.tsx index cf670b0e4f..8586920df6 100644 --- a/packages/iris-grid/src/sidebar/CustomColumnBuilder.test.tsx +++ b/packages/iris-grid/src/sidebar/CustomColumnBuilder.test.tsx @@ -100,7 +100,10 @@ test('Ignores deleted formulas on save', async () => { // There is an issue with populating the custom columns and then editing the existing column // RTL/monaco aren't playing nicely and it won't edit the existing text // This test instead creates the new text, saves, then removes it to test the same behavior - jest.useFakeTimers(); + jest.useFakeTimers({ + // Monaco makes a call to performance.mark(), so we need to leave it intact + doNotFake: ['performance'], + }); const user = userEvent.setup({ delay: null }); const model = irisGridTestUtils.makeModel(); const mockSave = jest.fn(() =>