Skip to content

Commit

Permalink
fix: Upgrade Monaco to ^0.41.0 (#1448)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bmingles committed Aug 16, 2023
1 parent 6d2cb06 commit 1120c2b
Show file tree
Hide file tree
Showing 13 changed files with 178 additions and 129 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Expand Down
22 changes: 22 additions & 0 deletions jest.setup.ts
Original file line number Diff line number Diff line change
@@ -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)) {
Expand Down Expand Up @@ -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<ResizeObserver>();
},
});

Object.defineProperty(window, 'TextDecoder', {
value: TextDecoder,
});

Object.defineProperty(window, 'TextEncoder', {
value: TextEncoder,
});

Object.defineProperty(document, 'fonts', {
value: {
ready: Promise.resolve(),
Expand Down
31 changes: 17 additions & 14 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion packages/code-studio/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
5 changes: 3 additions & 2 deletions packages/console/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
10 changes: 4 additions & 6 deletions packages/console/src/ConsoleInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand Down
55 changes: 25 additions & 30 deletions packages/console/src/log/LogView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -226,36 +227,30 @@ class LogView extends PureComponent<LogViewProps, LogViewState> {
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 {
Expand Down
55 changes: 55 additions & 0 deletions packages/console/src/monaco/MonacoUtils.test.ts
Original file line number Diff line number Diff line change
@@ -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<typeof Shortcut>[0] = {
id: 'Single key',
name: '',
Expand Down Expand Up @@ -45,6 +48,11 @@ const MULTI_MOD_PARAMS: ConstructorParameters<typeof Shortcut>[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;
Expand Down Expand Up @@ -151,6 +159,23 @@ describe('Mac shortcuts', () => {
});
});

describe('disableKeyBindings', () => {
const editor = createMockProxy<monaco.editor.IStandaloneCodeEditor>();

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;
Expand All @@ -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,
});
}
}
);
});
Loading

0 comments on commit 1120c2b

Please sign in to comment.