Merge branch '288312-editor-lite-extension-options' into 'master'

Support extensions as configurable ES6 classes

See merge request gitlab-org/gitlab!49813

app/assets/javascripts/blob_edit/edit_blob.js

@@ -5,7 +5,7 @@ import { BLOB_EDITOR_ERROR, BLOB_PREVIEW_ERROR } from './constants';
 import TemplateSelectorMediator from '../blob/file_template_mediator';
 import { addEditorMarkdownListeners } from '~/lib/utils/text_markdown';
 import EditorLite from '~/editor/editor_lite';
-import FileTemplateExtension from '~/editor/editor_file_template_ext';
+import { FileTemplateExtension } from '~/editor/editor_file_template_ext';
 
 export default class EditBlob {
   // The options object has:
@@ -16,11 +16,11 @@ export default class EditBlob {
 
     if (this.options.isMarkdown) {
       import('~/editor/editor_markdown_ext')
-        .then(MarkdownExtension => {
-          this.editor.use(MarkdownExtension.default);
+        .then(({ EditorMarkdownExtension: MarkdownExtension } = {}) => {
+          this.editor.use(new MarkdownExtension());
           addEditorMarkdownListeners(this.editor);
         })
-        .catch(() => createFlash(BLOB_EDITOR_ERROR));
+        .catch(e => createFlash(`${BLOB_EDITOR_ERROR}: ${e}`));
     }
 
     this.initModePanesAndLinks();
@@ -42,7 +42,7 @@ export default class EditBlob {
       blobPath: fileNameEl.value,
       blobContent: editorEl.innerText,
     });
-    this.editor.use(FileTemplateExtension);
+    this.editor.use(new FileTemplateExtension());
 
     fileNameEl.addEventListener('change', () => {
       this.editor.updateModelLanguage(fileNameEl.value);

app/assets/javascripts/editor/constants.js

@@ -6,3 +6,7 @@ export const EDITOR_LITE_INSTANCE_ERROR_NO_EL = __(
 
 export const URI_PREFIX = 'gitlab';
 export const CONTENT_UPDATE_DEBOUNCE = 250;
+
+export const ERROR_INSTANCE_REQUIRED_FOR_EXTENSION = __(
+  'Editor Lite instance is required to set up an extension.',
+);

app/assets/javascripts/editor/editor_file_template_ext.js

 import { Position } from 'monaco-editor';
+import { EditorLiteExtension } from './editor_lite_extension_base';
 
-export default {
+export class FileTemplateExtension extends EditorLiteExtension {
   navigateFileStart() {
     this.setPosition(new Position(1, 1));
-  },
-};
+  }
+}

app/assets/javascripts/editor/editor_lite.js

@@ -8,7 +8,7 @@ import { clearDomElement } from './utils';
 import { EDITOR_LITE_INSTANCE_ERROR_NO_EL, URI_PREFIX } from './constants';
 import { uuids } from '~/diffs/utils/uuids';
 
-export default class Editor {
+export default class EditorLite {
   constructor(options = {}) {
     this.instances = [];
     this.options = {
@@ -17,7 +17,7 @@ export default class Editor {
       ...options,
     };
 
-    Editor.setupMonacoTheme();
+    EditorLite.setupMonacoTheme();
 
     registerLanguages(...languages);
   }
@@ -54,12 +54,25 @@ export default class Editor {
     extensionsArray.forEach(ext => {
       const prefix = ext.includes('/') ? '' : 'editor/';
       const trimmedExt = ext.replace(/^\//, '').trim();
-      Editor.pushToImportsArray(promises, `~/${prefix}${trimmedExt}`);
+      EditorLite.pushToImportsArray(promises, `~/${prefix}${trimmedExt}`);
     });
 
     return Promise.all(promises);
   }
 
+  static mixIntoInstance(source, inst) {
+    if (!inst) {
+      return;
+    }
+    const isClassInstance = source.constructor.prototype !== Object.prototype;
+    const sanitizedSource = isClassInstance ? source.constructor.prototype : source;
+    Object.getOwnPropertyNames(sanitizedSource).forEach(prop => {
+      if (prop !== 'constructor') {
+        Object.assign(inst, { [prop]: source[prop] });
+      }
+    });
+  }
+
   /**
    * Creates a monaco instance with the given options.
    *
@@ -101,10 +114,10 @@ export default class Editor {
       this.instances.splice(index, 1);
       model.dispose();
     });
-    instance.updateModelLanguage = path => Editor.updateModelLanguage(path, instance);
+    instance.updateModelLanguage = path => EditorLite.updateModelLanguage(path, instance);
     instance.use = args => this.use(args, instance);
 
-    Editor.loadExtensions(extensions, instance)
+    EditorLite.loadExtensions(extensions, instance)
       .then(modules => {
         if (modules) {
           modules.forEach(module => {
@@ -129,10 +142,17 @@ export default class Editor {
 
   use(exts = [], instance = null) {
     const extensions = Array.isArray(exts) ? exts : [exts];
+    const initExtensions = inst => {
+      extensions.forEach(extension => {
+        EditorLite.mixIntoInstance(extension, inst);
+      });
+    };
     if (instance) {
-      Object.assign(instance, ...extensions);
+      initExtensions(instance);
     } else {
-      this.instances.forEach(inst => Object.assign(inst, ...extensions));
+      this.instances.forEach(inst => {
+        initExtensions(inst);
+      });
     }
   }
 }

app/assets/javascripts/editor/editor_lite_extension_base.js

+import { ERROR_INSTANCE_REQUIRED_FOR_EXTENSION } from './constants';
+
+export class EditorLiteExtension {
+  constructor({ instance, ...options } = {}) {
+    if (instance) {
+      Object.assign(instance, options);
+    } else if (Object.entries(options).length) {
+      throw new Error(ERROR_INSTANCE_REQUIRED_FOR_EXTENSION);
+    }
+  }
+}

app/assets/javascripts/editor/editor_markdown_ext.js

-export default {
+import { EditorLiteExtension } from './editor_lite_extension_base';
+
+export class EditorMarkdownExtension extends EditorLiteExtension {
   getSelectedText(selection = this.getSelection()) {
     const { startLineNumber, endLineNumber, startColumn, endColumn } = selection;
     const valArray = this.getValue().split('\n');
@@ -18,19 +20,19 @@ export default {
         : [startLineText, endLineText].join('\n');
     }
     return text;
-  },
+  }
 
   replaceSelectedText(text, select = undefined) {
     const forceMoveMarkers = !select;
     this.executeEdits('', [{ range: this.getSelection(), text, forceMoveMarkers }]);
-  },
+  }
 
   moveCursor(dx = 0, dy = 0) {
     const pos = this.getPosition();
     pos.column += dx;
     pos.lineNumber += dy;
     this.setPosition(pos);
-  },
+  }
 
   /**
    * Adjust existing selection to select text within the original selection.
@@ -91,5 +93,5 @@ export default {
       .setEndPosition(newEndLineNumber, newEndColumn);
 
     this.setSelection(newSelection);
-  },
-};
+  }
+}

changelogs/unreleased/288312-editor-lite-extension-options.yml

+---
+title: Support extensions as configurable ES6 classes in Editor Lite
+merge_request: 49813
+author:
+type: added

locale/gitlab.pot

@@ -10209,6 +10209,9 @@ msgstr ""
 msgid "Editing"
 msgstr ""
 
+msgid "Editor Lite instance is required to set up an extension."
+msgstr ""
+
 msgid "Elasticsearch AWS IAM credentials"
 msgstr ""
 

spec/frontend/blob_edit/edit_blob_spec.js

 import waitForPromises from 'helpers/wait_for_promises';
 import EditBlob from '~/blob_edit/edit_blob';
 import EditorLite from '~/editor/editor_lite';
-import MarkdownExtension from '~/editor/editor_markdown_ext';
-import FileTemplateExtension from '~/editor/editor_file_template_ext';
+import { EditorMarkdownExtension } from '~/editor/editor_markdown_ext';
+import { FileTemplateExtension } from '~/editor/editor_file_template_ext';
 
 jest.mock('~/editor/editor_lite');
 jest.mock('~/editor/editor_markdown_ext');
+jest.mock('~/editor/editor_file_template_ext');
 
 describe('Blob Editing', () => {
   const useMock = jest.fn();
@@ -20,6 +21,10 @@ describe('Blob Editing', () => {
     );
     jest.spyOn(EditorLite.prototype, 'createInstance').mockReturnValue(mockInstance);
   });
+  afterEach(() => {
+    EditorMarkdownExtension.mockClear();
+    FileTemplateExtension.mockClear();
+  });
 
   const editorInst = isMarkdown => {
     return new EditBlob({
@@ -34,20 +39,20 @@ describe('Blob Editing', () => {
 
   it('loads FileTemplateExtension by default', async () => {
     await initEditor();
-    expect(useMock).toHaveBeenCalledWith(FileTemplateExtension);
+    expect(FileTemplateExtension).toHaveBeenCalledTimes(1);
   });
 
   describe('Markdown', () => {
     it('does not load MarkdownExtension by default', async () => {
       await initEditor();
-      expect(useMock).not.toHaveBeenCalledWith(MarkdownExtension);
+      expect(EditorMarkdownExtension).not.toHaveBeenCalled();
     });
 
     it('loads MarkdownExtension only for the markdown files', async () => {
       await initEditor(true);
       expect(useMock).toHaveBeenCalledTimes(2);
-      expect(useMock).toHaveBeenNthCalledWith(1, FileTemplateExtension);
-      expect(useMock).toHaveBeenNthCalledWith(2, MarkdownExtension);
+      expect(FileTemplateExtension).toHaveBeenCalledTimes(1);
+      expect(EditorMarkdownExtension).toHaveBeenCalledTimes(1);
     });
   });
 });

spec/frontend/editor/editor_lite_extension_base_spec.js

+import { ERROR_INSTANCE_REQUIRED_FOR_EXTENSION } from '~/editor/constants';
+import { EditorLiteExtension } from '~/editor/editor_lite_extension_base';
+
+describe('The basis for an Editor Lite extension', () => {
+  let ext;
+  const defaultOptions = { foo: 'bar' };
+
+  it.each`
+    description                                                     | instance     | options
+    ${'accepts configuration options and instance'}                 | ${{}}        | ${defaultOptions}
+    ${'leaves instance intact if no options are passed'}            | ${{}}        | ${undefined}
+    ${'does not fail if both instance and the options are omitted'} | ${undefined} | ${undefined}
+    ${'throws if only options are passed'}                          | ${undefined} | ${defaultOptions}
+  `('$description', ({ instance, options } = {}) => {
+    const originalInstance = { ...instance };
+
+    if (instance) {
+      if (options) {
+        Object.entries(options).forEach(prop => {
+          expect(instance[prop]).toBeUndefined();
+        });
+        // Both instance and options are passed
+        ext = new EditorLiteExtension({ instance, ...options });
+        Object.entries(options).forEach(([prop, value]) => {
+          expect(ext[prop]).toBeUndefined();
+          expect(instance[prop]).toBe(value);
+        });
+      } else {
+        ext = new EditorLiteExtension({ instance });
+        expect(instance).toEqual(originalInstance);
+      }
+    } else if (options) {
+      // Options are passed without instance
+      expect(() => {
+        ext = new EditorLiteExtension({ ...options });
+      }).toThrow(ERROR_INSTANCE_REQUIRED_FOR_EXTENSION);
+    } else {
+      // Neither options nor instance are passed
+      expect(() => {
+        ext = new EditorLiteExtension();
+      }).not.toThrow();
+    }
+  });
+});

spec/frontend/editor/editor_lite_spec.js

+/* eslint-disable max-classes-per-file */
 import { editor as monacoEditor, languages as monacoLanguages, Uri } from 'monaco-editor';
 import waitForPromises from 'helpers/wait_for_promises';
 import Editor from '~/editor/editor_lite';
+import { EditorLiteExtension } from '~/editor/editor_lite_extension_base';
 import { DEFAULT_THEME, themes } from '~/ide/lib/themes';
 import { EDITOR_LITE_INSTANCE_ERROR_NO_EL, URI_PREFIX } from '~/editor/constants';
 
@@ -242,17 +244,53 @@ describe('Base editor', () => {
 
   describe('extensions', () => {
     let instance;
-    const foo1 = jest.fn();
-    const foo2 = jest.fn();
-    const bar = jest.fn();
-    const MyExt1 = {
-      foo: foo1,
+    const alphaRes = jest.fn();
+    const betaRes = jest.fn();
+    const fooRes = jest.fn();
+    const barRes = jest.fn();
+    class AlphaClass {
+      constructor() {
+        this.res = alphaRes;
+      }
+      alpha() {
+        return this?.nonExistentProp || alphaRes;
+      }
+    }
+    class BetaClass {
+      beta() {
+        return this?.nonExistentProp || betaRes;
+      }
+    }
+    class WithStaticMethod {
+      constructor({ instance: inst, ...options } = {}) {
+        Object.assign(inst, options);
+      }
+      static computeBoo(a) {
+        return a + 1;
+      }
+      boo() {
+        return WithStaticMethod.computeBoo(this.base);
+      }
+    }
+    class WithStaticMethodExtended extends EditorLiteExtension {
+      static computeBoo(a) {
+        return a + 1;
+      }
+      boo() {
+        return WithStaticMethodExtended.computeBoo(this.base);
+      }
+    }
+    const AlphaExt = new AlphaClass();
+    const BetaExt = new BetaClass();
+    const FooObjExt = {
+      foo() {
+        return fooRes;
+      },
     };
-    const MyExt2 = {
-      bar,
-    };
-    const MyExt3 = {
-      foo: foo2,
+    const BarObjExt = {
+      bar() {
+        return barRes;
+      },
     };
 
     describe('basic functionality', () => {
@@ -260,13 +298,6 @@ describe('Base editor', () => {
         instance = editor.createInstance({ el: editorEl, blobPath, blobContent });
       });
 
-      it('is extensible with the extensions', () => {
-        expect(instance.foo).toBeUndefined();
-
-        instance.use(MyExt1);
-        expect(instance.foo).toEqual(foo1);
-      });
-
       it('does not fail if no extensions supplied', () => {
         const spy = jest.spyOn(global.console, 'error');
         instance.use();
@@ -274,24 +305,80 @@ describe('Base editor', () => {
         expect(spy).not.toHaveBeenCalled();
       });
 
-      it('is extensible with multiple extensions', () => {
-        expect(instance.foo).toBeUndefined();
-        expect(instance.bar).toBeUndefined();
+      it("does not extend instance with extension's constructor", () => {
+        expect(instance.constructor).toBeDefined();
+        const { constructor } = instance;
+
+        expect(AlphaExt.constructor).toBeDefined();
+        expect(AlphaExt.constructor).not.toEqual(constructor);
+
+        instance.use(AlphaExt);
+        expect(instance.constructor).toBe(constructor);
+      });
+
+      it.each`
+        type                                        | extensions                | methods              | expectations
+        ${'ES6 classes'}                            | ${AlphaExt}               | ${['alpha']}         | ${[alphaRes]}
+        ${'multiple ES6 classes'}                   | ${[AlphaExt, BetaExt]}    | ${['alpha', 'beta']} | ${[alphaRes, betaRes]}
+        ${'simple objects'}                         | ${FooObjExt}              | ${['foo']}           | ${[fooRes]}
+        ${'multiple simple objects'}                | ${[FooObjExt, BarObjExt]} | ${['foo', 'bar']}    | ${[fooRes, barRes]}
+        ${'combination of ES6 classes and objects'} | ${[AlphaExt, BarObjExt]}  | ${['alpha', 'bar']}  | ${[alphaRes, barRes]}
+      `('is extensible with $type', ({ extensions, methods, expectations } = {}) => {
+        methods.forEach(method => {
+          expect(instance[method]).toBeUndefined();
+        });
 
-        instance.use([MyExt1, MyExt2]);
+        instance.use(extensions);
 
-        expect(instance.foo).toEqual(foo1);
-        expect(instance.bar).toEqual(bar);
+        methods.forEach(method => {
+          expect(instance[method]).toBeDefined();
+        });
+
+        expectations.forEach((expectation, i) => {
+          expect(instance[methods[i]].call()).toEqual(expectation);
+        });
       });
 
+      it('does not extend instance with private data of an extension', () => {
+        const ext = new WithStaticMethod({ instance });
+        ext.staticMethod = () => {
+          return 'foo';
+        };
+        ext.staticProp = 'bar';
+
+        expect(instance.boo).toBeUndefined();
+        expect(instance.staticMethod).toBeUndefined();
+        expect(instance.staticProp).toBeUndefined();
+
+        instance.use(ext);
+
+        expect(instance.boo).toBeDefined();
+        expect(instance.staticMethod).toBeUndefined();
+        expect(instance.staticProp).toBeUndefined();
+      });
+
+      it.each([WithStaticMethod, WithStaticMethodExtended])(
+        'properly resolves data for an extension with private data',
+        ExtClass => {
+          const base = 1;
+          expect(instance.base).toBeUndefined();
+          expect(instance.boo).toBeUndefined();
+
+          const ext = new ExtClass({ instance, base });
+
+          instance.use(ext);
+          expect(instance.base).toBe(1);
+          expect(instance.boo()).toBe(2);
+        },
+      );
+
       it('uses the last definition of a method in case of an overlap', () => {
-        instance.use([MyExt1, MyExt2, MyExt3]);
-        expect(instance).toEqual(
-          expect.objectContaining({
-            foo: foo2,
-            bar,
-          }),
-        );
+        const FooObjExt2 = { foo: 'foo2' };
+        instance.use([FooObjExt, BarObjExt, FooObjExt2]);
+        expect(instance).toMatchObject({
+          foo: 'foo2',
+          ...BarObjExt,
+        });
       });
 
       it('correctly resolves references withing extensions', () => {
@@ -396,15 +483,15 @@ describe('Base editor', () => {
       });
 
       it('extends all instances if no specific instance is passed', () => {
-        editor.use(MyExt1);
-        expect(inst1.foo).toEqual(foo1);
-        expect(inst2.foo).toEqual(foo1);
+        editor.use(AlphaExt);
+        expect(inst1.alpha()).toEqual(alphaRes);
+        expect(inst2.alpha()).toEqual(alphaRes);
       });
 
       it('extends specific instance if it has been passed', () => {
-        editor.use(MyExt1, inst2);
-        expect(inst1.foo).toBeUndefined();
-        expect(inst2.foo).toEqual(foo1);
+        editor.use(AlphaExt, inst2);
+        expect(inst1.alpha).toBeUndefined();
+        expect(inst2.alpha()).toEqual(alphaRes);
       });
     });
   });

spec/frontend/editor/editor_markdown_ext_spec.js

 import { Range, Position } from 'monaco-editor';
 import EditorLite from '~/editor/editor_lite';
-import EditorMarkdownExtension from '~/editor/editor_markdown_ext';
+import { EditorMarkdownExtension } from '~/editor/editor_markdown_ext';
 
 describe('Markdown Extension for Editor Lite', () => {
   let editor;
@@ -31,7 +31,7 @@ describe('Markdown Extension for Editor Lite', () => {
       blobPath: filePath,
       blobContent: text,
     });
-    editor.use(EditorMarkdownExtension);
+    editor.use(new EditorMarkdownExtension());
   });
 
   afterEach(() => {