From 55a03d462cc9cd45f66602ea8a82e4a76b1d262d Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Wed, 17 Nov 2021 00:24:54 +0100 Subject: [PATCH 01/25] Switched to new Source Editor Instance --- .../javascripts/editor/source_editor.js | 107 +++---- spec/frontend/editor/source_editor_spec.js | 276 ++---------------- 2 files changed, 65 insertions(+), 318 deletions(-) diff --git a/app/assets/javascripts/editor/source_editor.js b/app/assets/javascripts/editor/source_editor.js index 26fbd1f4d8ab5e..4770357c3386e3 100644 --- a/app/assets/javascripts/editor/source_editor.js +++ b/app/assets/javascripts/editor/source_editor.js @@ -1,4 +1,5 @@ import { editor as monacoEditor, Uri } from 'monaco-editor'; +import { waitForCSSLoaded } from '~/helpers/startup_css_helper'; import { defaultEditorOptions } from '~/ide/lib/editor_options'; import languages from '~/ide/lib/languages'; import { registerLanguages } from '~/ide/utils'; @@ -11,10 +12,35 @@ import { EDITOR_TYPE_DIFF, } from './constants'; import { clearDomElement, setupEditorTheme, getBlobLanguage } from './utils'; +import EditorInstance from './source_editor_instance'; + +const instanceRemoveFromRegistry = (editor, instance) => { + const index = editor.instances.findIndex((inst) => inst === instance); + editor.instances.splice(index, 1); +}; + +const instanceDisposeModels = (editor, instance, model) => { + const instanceModel = instance.getModel() || model; + if (!instanceModel) { + return; + } + if (instance.getEditorType() === EDITOR_TYPE_DIFF) { + const { original, modified } = instanceModel; + if (original) { + original.dispose(); + } + if (modified) { + modified.dispose(); + } + } else { + instanceModel.dispose(); + } +}; export default class SourceEditor { constructor(options = {}) { this.instances = []; + this.extensionsStore = new Map(); this.options = { extraEditorClassName: 'gl-source-editor', ...defaultEditorOptions, @@ -26,19 +52,6 @@ export default class SourceEditor { registerLanguages(...languages); } - 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] }); - } - }); - } - static prepareInstance(el) { if (!el) { throw new Error(SOURCE_EDITOR_INSTANCE_ERROR_NO_EL); @@ -78,63 +91,6 @@ export default class SourceEditor { return diffModel; } - static convertMonacoToELInstance = (inst) => { - const sourceEditorInstanceAPI = { - updateModelLanguage: (path) => { - return SourceEditor.instanceUpdateLanguage(inst, path); - }, - use: (exts = []) => { - return SourceEditor.instanceApplyExtension(inst, exts); - }, - }; - const handler = { - get(target, prop, receiver) { - if (Reflect.has(sourceEditorInstanceAPI, prop)) { - return sourceEditorInstanceAPI[prop]; - } - return Reflect.get(target, prop, receiver); - }, - }; - return new Proxy(inst, handler); - }; - - static instanceUpdateLanguage(inst, path) { - const lang = getBlobLanguage(path); - const model = inst.getModel(); - return monacoEditor.setModelLanguage(model, lang); - } - - static instanceApplyExtension(inst, exts = []) { - const extensions = [].concat(exts); - extensions.forEach((extension) => { - SourceEditor.mixIntoInstance(extension, inst); - }); - return inst; - } - - static instanceRemoveFromRegistry(editor, instance) { - const index = editor.instances.findIndex((inst) => inst === instance); - editor.instances.splice(index, 1); - } - - static instanceDisposeModels(editor, instance, model) { - const instanceModel = instance.getModel() || model; - if (!instanceModel) { - return; - } - if (instance.getEditorType() === EDITOR_TYPE_DIFF) { - const { original, modified } = instanceModel; - if (original) { - original.dispose(); - } - if (modified) { - modified.dispose(); - } - } else { - instanceModel.dispose(); - } - } - /** * Creates a monaco instance with the given options. * @@ -156,13 +112,18 @@ export default class SourceEditor { SourceEditor.prepareInstance(el); const createEditorFn = isDiff ? 'createDiffEditor' : 'create'; - const instance = SourceEditor.convertMonacoToELInstance( + const instance = new EditorInstance( monacoEditor[createEditorFn].call(this, el, { ...this.options, ...instanceOptions, }), + this.extensionsStore, ); + waitForCSSLoaded(() => { + instance.layout(); + }); + let model; if (instanceOptions.model !== null) { model = SourceEditor.createEditorModel({ @@ -176,8 +137,8 @@ export default class SourceEditor { } instance.onDidDispose(() => { - SourceEditor.instanceRemoveFromRegistry(this, instance); - SourceEditor.instanceDisposeModels(this, instance, model); + instanceRemoveFromRegistry(this, instance); + instanceDisposeModels(this, instance, model); }); this.instances.push(instance); diff --git a/spec/frontend/editor/source_editor_spec.js b/spec/frontend/editor/source_editor_spec.js index f1b887b2dc1935..a53eb6fc173655 100644 --- a/spec/frontend/editor/source_editor_spec.js +++ b/spec/frontend/editor/source_editor_spec.js @@ -1,11 +1,9 @@ -/* eslint-disable max-classes-per-file */ import { editor as monacoEditor, languages as monacoLanguages } from 'monaco-editor'; import { SOURCE_EDITOR_INSTANCE_ERROR_NO_EL, URI_PREFIX, EDITOR_READY_EVENT, } from '~/editor/constants'; -import { SourceEditorExtension } from '~/editor/extensions/source_editor_extension_base'; import SourceEditor from '~/editor/source_editor'; import { DEFAULT_THEME, themes } from '~/ide/lib/themes'; import { joinPaths } from '~/lib/utils/url_utility'; @@ -18,7 +16,6 @@ describe('Base editor', () => { const blobContent = 'Foo Bar'; const blobPath = 'test.md'; const blobGlobalId = 'snippet_777'; - const fakeModel = { foo: 'bar', dispose: jest.fn() }; beforeEach(() => { setFixtures('
'); @@ -51,16 +48,6 @@ describe('Base editor', () => { describe('instance of the Source Editor', () => { let modelSpy; let instanceSpy; - const setModel = jest.fn(); - const dispose = jest.fn(); - const mockModelReturn = (res = fakeModel) => { - modelSpy = jest.spyOn(monacoEditor, 'createModel').mockImplementation(() => res); - }; - const mockDecorateInstance = (decorations = {}) => { - jest.spyOn(SourceEditor, 'convertMonacoToELInstance').mockImplementation((inst) => { - return Object.assign(inst, decorations); - }); - }; beforeEach(() => { modelSpy = jest.spyOn(monacoEditor, 'createModel'); @@ -72,46 +59,38 @@ describe('Base editor', () => { }); it('throws an error if no dom element is supplied', () => { - mockDecorateInstance(); - expect(() => { + const create = () => { editor.createInstance(); - }).toThrow(SOURCE_EDITOR_INSTANCE_ERROR_NO_EL); + }; + expect(create).toThrow(SOURCE_EDITOR_INSTANCE_ERROR_NO_EL); expect(modelSpy).not.toHaveBeenCalled(); expect(instanceSpy).not.toHaveBeenCalled(); - expect(SourceEditor.convertMonacoToELInstance).not.toHaveBeenCalled(); }); - it('creates model to be supplied to Monaco editor', () => { - mockModelReturn(); - mockDecorateInstance({ - setModel, - }); - editor.createInstance(defaultArguments); + it('creates model and attaches it to the instance', () => { + jest.spyOn(monacoEditor, 'createModel'); + const instance = editor.createInstance(defaultArguments); - expect(modelSpy).toHaveBeenCalledWith( + expect(monacoEditor.createModel).toHaveBeenCalledWith( blobContent, undefined, expect.objectContaining({ path: uriFilePath, }), ); - expect(setModel).toHaveBeenCalledWith(fakeModel); + expect(instance.getModel().getValue()).toEqual(defaultArguments.blobContent); }); it('does not create a model automatically if model is passed as `null`', () => { - mockDecorateInstance({ - setModel, - }); - editor.createInstance({ ...defaultArguments, model: null }); - expect(modelSpy).not.toHaveBeenCalled(); - expect(setModel).not.toHaveBeenCalled(); + const instance = editor.createInstance({ ...defaultArguments, model: null }); + expect(instance.getModel()).toBeNull(); }); it('initializes the instance on a supplied DOM node', () => { editor.createInstance({ el: editorEl }); - expect(editor.editorEl).not.toBe(null); + expect(editor.editorEl).not.toBeNull(); expect(instanceSpy).toHaveBeenCalledWith(editorEl, expect.anything()); }); @@ -142,32 +121,26 @@ describe('Base editor', () => { }); it('disposes instance when the global editor is disposed', () => { - mockDecorateInstance({ - dispose, - }); - editor.createInstance(defaultArguments); + const instance = editor.createInstance(defaultArguments); + instance.dispose = jest.fn(); - expect(dispose).not.toHaveBeenCalled(); + expect(instance.dispose).not.toHaveBeenCalled(); editor.dispose(); - expect(dispose).toHaveBeenCalled(); + expect(instance.dispose).toHaveBeenCalled(); }); it("removes the disposed instance from the global editor's storage and disposes the associated model", () => { - mockModelReturn(); - mockDecorateInstance({ - setModel, - }); const instance = editor.createInstance(defaultArguments); expect(editor.instances).toHaveLength(1); - expect(fakeModel.dispose).not.toHaveBeenCalled(); + expect(instance.getModel()).not.toBeNull(); instance.dispose(); expect(editor.instances).toHaveLength(0); - expect(fakeModel.dispose).toHaveBeenCalled(); + expect(instance.getModel()).toBeNull(); }); }); @@ -213,26 +186,17 @@ describe('Base editor', () => { }); it('correctly disposes the diff editor model', () => { - const modifiedModel = fakeModel; - const originalModel = { ...fakeModel }; - mockDecorateInstance({ - getModel: jest.fn().mockReturnValue({ - original: originalModel, - modified: modifiedModel, - }), - }); - const instance = editor.createDiffInstance({ ...defaultArguments, blobOriginalContent }); expect(editor.instances).toHaveLength(1); - expect(originalModel.dispose).not.toHaveBeenCalled(); - expect(modifiedModel.dispose).not.toHaveBeenCalled(); + expect(instance.getOriginalEditor().getModel()).not.toBeNull(); + expect(instance.getModifiedEditor().getModel()).not.toBeNull(); instance.dispose(); expect(editor.instances).toHaveLength(0); - expect(originalModel.dispose).toHaveBeenCalled(); - expect(modifiedModel.dispose).toHaveBeenCalled(); + expect(instance.getOriginalEditor().getModel()).toBeNull(); + expect(instance.getModifiedEditor().getModel()).toBeNull(); }); }); }); @@ -354,196 +318,18 @@ describe('Base editor', () => { expect(instance.getValue()).toBe(blobContent); }); - it('is capable of changing the language of the model', () => { - // ignore warnings and errors Monaco posts during setup - // (due to being called from Jest/Node.js environment) - jest.spyOn(console, 'warn').mockImplementation(() => {}); - jest.spyOn(console, 'error').mockImplementation(() => {}); - - const blobRenamedPath = 'test.js'; - - expect(instance.getModel().getLanguageIdentifier().language).toBe('markdown'); - instance.updateModelLanguage(blobRenamedPath); - - expect(instance.getModel().getLanguageIdentifier().language).toBe('javascript'); - }); - - it('falls back to plaintext if there is no language associated with an extension', () => { - const blobRenamedPath = 'test.myext'; - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}); - - instance.updateModelLanguage(blobRenamedPath); - - expect(spy).not.toHaveBeenCalled(); - expect(instance.getModel().getLanguageIdentifier().language).toBe('plaintext'); - }); - }); - - describe('extensions', () => { - let instance; - 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 SourceEditorExtension { - 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 BarObjExt = { - bar() { - return barRes; - }, - }; - - describe('basic functionality', () => { - beforeEach(() => { - instance = editor.createInstance({ el: editorEl, blobPath, blobContent }); - }); - - it('does not fail if no extensions supplied', () => { - const spy = jest.spyOn(global.console, 'error'); - instance.use(); - - expect(spy).not.toHaveBeenCalled(); - }); - - 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(extensions); - - 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', () => { - const FooObjExt2 = { foo: 'foo2' }; - instance.use([FooObjExt, BarObjExt, FooObjExt2]); - expect(instance).toMatchObject({ - foo: 'foo2', - ...BarObjExt, - }); - }); - - it('correctly resolves references withing extensions', () => { - const FunctionExt = { - inst() { - return this; - }, - mod() { - return this.getModel(); - }, + it('emits the EDITOR_READY_EVENT event after setting up the instance', () => { + jest.spyOn(monacoEditor, 'create').mockImplementation(() => { + return { + setModel: jest.fn(), + onDidDispose: jest.fn(), }; - instance.use(FunctionExt); - expect(instance.inst()).toEqual(editor.instances[0]); - }); - - it('emits the EDITOR_READY_EVENT event after setting up the instance', () => { - jest.spyOn(monacoEditor, 'create').mockImplementation(() => { - return { - setModel: jest.fn(), - onDidDispose: jest.fn(), - }; - }); - const eventSpy = jest.fn(); - editorEl.addEventListener(EDITOR_READY_EVENT, eventSpy); - expect(eventSpy).not.toHaveBeenCalled(); - instance = editor.createInstance({ el: editorEl }); - expect(eventSpy).toHaveBeenCalled(); }); + const eventSpy = jest.fn(); + editorEl.addEventListener(EDITOR_READY_EVENT, eventSpy); + expect(eventSpy).not.toHaveBeenCalled(); + editor.createInstance({ el: editorEl }); + expect(eventSpy).toHaveBeenCalled(); }); }); -- GitLab From a731244a35af4f0c8104ed57fd9cb76aada6511c Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Wed, 17 Nov 2021 00:49:51 +0100 Subject: [PATCH 02/25] JSDoc documentation for the API --- .../javascripts/editor/source_editor.js | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/editor/source_editor.js b/app/assets/javascripts/editor/source_editor.js index 4770357c3386e3..57e2b0da565732 100644 --- a/app/assets/javascripts/editor/source_editor.js +++ b/app/assets/javascripts/editor/source_editor.js @@ -38,6 +38,10 @@ const instanceDisposeModels = (editor, instance, model) => { }; export default class SourceEditor { + /** + * Constructs a global editor. + * @param {Object} options - Monaco config options used to create the editor + */ constructor(options = {}) { this.instances = []; this.extensionsStore = new Map(); @@ -92,13 +96,16 @@ export default class SourceEditor { } /** - * Creates a monaco instance with the given options. - * - * @param {Object} options Options used to initialize monaco. - * @param {Element} options.el The element which will be used to create the monacoEditor. + * Creates a Source Editor Instance with the given options. + * @param {Object} options Options used to initialize the instance. + * @param {Element} options.el The element to attach the instance for. * @param {string} options.blobPath The path used as the URI of the model. Monaco uses the extension of this path to determine the language. * @param {string} options.blobContent The content to initialize the monacoEditor. + * @param {string} options.blobOriginalContent The original blob's content. Is used when creating a Diff Instance. * @param {string} options.blobGlobalId This is used to help globally identify monaco instances that are created with the same blobPath. + * @param {Boolean} options.isDiff Flag to enable creation of a Diff Instance? + * @param {...*} options.instanceOptions Configuration options used to instantiate an instance. + * @returns {EditorInstance} */ createInstance({ el = undefined, @@ -146,6 +153,11 @@ export default class SourceEditor { return instance; } + /** + * Create a Diff Instance + * @param {Object} args Options to be passed further down to createInstance() with the same signature + * @returns {EditorInstance} + */ createDiffInstance(args) { return this.createInstance({ ...args, @@ -153,6 +165,10 @@ export default class SourceEditor { }); } + /** + * Dispose global editor + * Automatically disposes all the instances registered for this editor + */ dispose() { this.instances.forEach((instance) => instance.dispose()); } -- GitLab From b24069247b49d6c64c6c4a24e6355689b65bbb48 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Thu, 25 Nov 2021 11:55:31 +0100 Subject: [PATCH 03/25] Refactored the base extension --- .../source_editor_extension_base.js | 99 +++++------ .../source_editor_extension_base_spec.js | 154 ++++++------------ spec/frontend/editor/source_editor_spec.js | 32 ++++ 3 files changed, 129 insertions(+), 156 deletions(-) diff --git a/app/assets/javascripts/editor/extensions/source_editor_extension_base.js b/app/assets/javascripts/editor/extensions/source_editor_extension_base.js index 03c68fed3b183d..34104b0febb683 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_extension_base.js +++ b/app/assets/javascripts/editor/extensions/source_editor_extension_base.js @@ -1,6 +1,5 @@ import { Range } from 'monaco-editor'; -import { waitForCSSLoaded } from '~/helpers/startup_css_helper'; -import { ERROR_INSTANCE_REQUIRED_FOR_EXTENSION, EDITOR_TYPE_CODE } from '../constants'; +import { EDITOR_TYPE_CODE } from '../constants'; const hashRegexp = new RegExp('#?L', 'g'); @@ -17,38 +16,40 @@ const createAnchor = (href) => { }; export class SourceEditorExtension { - constructor({ instance, ...options } = {}) { - if (instance) { - Object.assign(instance, options); - SourceEditorExtension.highlightLines(instance); - if (instance.getEditorType && instance.getEditorType() === EDITOR_TYPE_CODE) { - SourceEditorExtension.setupLineLinking(instance); - } - SourceEditorExtension.deferRerender(instance); - } else if (Object.entries(options).length) { - throw new Error(ERROR_INSTANCE_REQUIRED_FOR_EXTENSION); + // eslint-disable-next-line class-methods-use-this + onUse(instance) { + SourceEditorExtension.highlightLines(instance); + if (instance.getEditorType && instance.getEditorType() === EDITOR_TYPE_CODE) { + SourceEditorExtension.setupLineLinking(instance); } } - static deferRerender(instance) { - waitForCSSLoaded(() => { - instance.layout(); - }); + static onMouseMoveHandler(e) { + const target = e.target.element; + if (target.classList.contains('line-numbers')) { + const lineNum = e.target.position.lineNumber; + const hrefAttr = `#L${lineNum}`; + let el = target.querySelector('a'); + if (!el) { + el = createAnchor(hrefAttr); + target.appendChild(el); + } + } } - static removeHighlights(instance) { - Object.assign(instance, { - lineDecorations: instance.deltaDecorations(instance.lineDecorations || [], []), + static setupLineLinking(instance) { + instance.onMouseMove(SourceEditorExtension.onMouseMoveHandler); + instance.onMouseDown((e) => { + const isCorrectAnchor = e.target.element.classList.contains('link-anchor'); + if (!isCorrectAnchor) { + return; + } + if (instance.lineDecorations) { + instance.deltaDecorations(instance.lineDecorations, []); + } }); } - /** - * Returns a function that can only be invoked once between - * each browser screen repaint. - * @param {Object} instance - The Source Editor instance - * @param {Array} bounds - The [start, end] array with start - * and end coordinates for highlighting - */ static highlightLines(instance, bounds = null) { const [start, end] = bounds && Array.isArray(bounds) @@ -74,29 +75,29 @@ export class SourceEditorExtension { } } - static onMouseMoveHandler(e) { - const target = e.target.element; - if (target.classList.contains('line-numbers')) { - const lineNum = e.target.position.lineNumber; - const hrefAttr = `#L${lineNum}`; - let el = target.querySelector('a'); - if (!el) { - el = createAnchor(hrefAttr); - target.appendChild(el); - } - } - } + // eslint-disable-next-line class-methods-use-this + provides() { + return { + /** + * Removes existing line decorations and updates the reference on the instance + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance + */ + removeHighlights: (instance) => { + Object.assign(instance, { + lineDecorations: instance.deltaDecorations(instance.lineDecorations || [], []), + }); + }, - static setupLineLinking(instance) { - instance.onMouseMove(SourceEditorExtension.onMouseMoveHandler); - instance.onMouseDown((e) => { - const isCorrectAnchor = e.target.element.classList.contains('link-anchor'); - if (!isCorrectAnchor) { - return; - } - if (instance.lineDecorations) { - instance.deltaDecorations(instance.lineDecorations, []); - } - }); + /** + * Returns a function that can only be invoked once between + * each browser screen repaint. + * @param {Array} bounds - The [start, end] array with start + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance + * and end coordinates for highlighting + */ + highlightLines(instance, bounds = null) { + SourceEditorExtension.highlightLines(instance, bounds); + }, + }; } } diff --git a/spec/frontend/editor/source_editor_extension_base_spec.js b/spec/frontend/editor/source_editor_extension_base_spec.js index a0fb1178b3bf0f..6ec593ba8d2d45 100644 --- a/spec/frontend/editor/source_editor_extension_base_spec.js +++ b/spec/frontend/editor/source_editor_extension_base_spec.js @@ -1,34 +1,14 @@ import { Range } from 'monaco-editor'; import { useFakeRequestAnimationFrame } from 'helpers/fake_request_animation_frame'; import setWindowLocation from 'helpers/set_window_location_helper'; -import { - ERROR_INSTANCE_REQUIRED_FOR_EXTENSION, - EDITOR_TYPE_CODE, - EDITOR_TYPE_DIFF, -} from '~/editor/constants'; +import { EDITOR_TYPE_CODE, EDITOR_TYPE_DIFF } from '~/editor/constants'; import { SourceEditorExtension } from '~/editor/extensions/source_editor_extension_base'; - -jest.mock('~/helpers/startup_css_helper', () => { - return { - waitForCSSLoaded: jest.fn().mockImplementation((cb) => { - // We have to artificially put the callback's execution - // to the end of the current call stack to be able to - // test that the callback is called after waitForCSSLoaded. - // setTimeout with 0 delay does exactly that. - // Otherwise we might end up with false positive results - setTimeout(() => { - cb.apply(); - }, 0); - }), - }; -}); +import EditorInstance from '~/editor/source_editor_instance'; describe('The basis for an Source Editor extension', () => { const defaultLine = 3; - let ext; let event; - const defaultOptions = { foo: 'bar' }; const findLine = (num) => { return document.querySelector(`.line-numbers:nth-child(${num})`); }; @@ -49,6 +29,9 @@ describe('The basis for an Source Editor extension', () => { }, }; }; + const createInstance = (baseInstance = {}) => { + return new EditorInstance(baseInstance); + }; beforeEach(() => { setFixtures(generateLines()); @@ -59,95 +42,47 @@ describe('The basis for an Source Editor extension', () => { jest.clearAllMocks(); }); - describe('constructor', () => { - it('resets the layout in waitForCSSLoaded callback', async () => { - const instance = { - layout: jest.fn(), - }; - ext = new SourceEditorExtension({ instance }); - expect(instance.layout).not.toHaveBeenCalled(); - - // We're waiting for the waitForCSSLoaded mock to kick in - await jest.runOnlyPendingTimers(); + describe('onUse callback', () => { + it('initializes the line highlighting', () => { + const instance = createInstance(); + const spy = jest.spyOn(SourceEditorExtension, 'highlightLines'); - expect(instance.layout).toHaveBeenCalled(); + instance.use({ definition: SourceEditorExtension }); + expect(spy).toHaveBeenCalled(); }); 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 } = {}) => { - SourceEditorExtension.deferRerender = jest.fn(); - const originalInstance = { ...instance }; - - if (instance) { - if (options) { - Object.entries(options).forEach((prop) => { - expect(instance[prop]).toBeUndefined(); - }); - // Both instance and options are passed - ext = new SourceEditorExtension({ instance, ...options }); - Object.entries(options).forEach(([prop, value]) => { - expect(ext[prop]).toBeUndefined(); - expect(instance[prop]).toBe(value); - }); + description | instanceType | shouldBeCalled + ${'Sets up'} | ${EDITOR_TYPE_CODE} | ${true} + ${'Does not set up'} | ${EDITOR_TYPE_DIFF} | ${false} + `( + '$description the line linking for $instanceType instance', + ({ instanceType, shouldBeCalled }) => { + const instance = createInstance({ + getEditorType: jest.fn().mockReturnValue(instanceType), + onMouseMove: jest.fn(), + onMouseDown: jest.fn(), + }); + const spy = jest.spyOn(SourceEditorExtension, 'setupLineLinking'); + + instance.use({ definition: SourceEditorExtension }); + if (shouldBeCalled) { + expect(spy).toHaveBeenCalledWith(instance); } else { - ext = new SourceEditorExtension({ instance }); - expect(instance).toEqual(originalInstance); + expect(spy).not.toHaveBeenCalled(); } - } else if (options) { - // Options are passed without instance - expect(() => { - ext = new SourceEditorExtension({ ...options }); - }).toThrow(ERROR_INSTANCE_REQUIRED_FOR_EXTENSION); - } else { - // Neither options nor instance are passed - expect(() => { - ext = new SourceEditorExtension(); - }).not.toThrow(); - } - }); - - it('initializes the line highlighting', () => { - SourceEditorExtension.deferRerender = jest.fn(); - const spy = jest.spyOn(SourceEditorExtension, 'highlightLines'); - ext = new SourceEditorExtension({ instance: {} }); - expect(spy).toHaveBeenCalled(); - }); - - it('sets up the line linking for code instance', () => { - SourceEditorExtension.deferRerender = jest.fn(); - const spy = jest.spyOn(SourceEditorExtension, 'setupLineLinking'); - const instance = { - getEditorType: jest.fn().mockReturnValue(EDITOR_TYPE_CODE), - onMouseMove: jest.fn(), - onMouseDown: jest.fn(), - }; - ext = new SourceEditorExtension({ instance }); - expect(spy).toHaveBeenCalledWith(instance); - }); - - it('does not set up the line linking for diff instance', () => { - SourceEditorExtension.deferRerender = jest.fn(); - const spy = jest.spyOn(SourceEditorExtension, 'setupLineLinking'); - const instance = { - getEditorType: jest.fn().mockReturnValue(EDITOR_TYPE_DIFF), - }; - ext = new SourceEditorExtension({ instance }); - expect(spy).not.toHaveBeenCalled(); - }); + }, + ); }); describe('highlightLines', () => { const revealSpy = jest.fn(); const decorationsSpy = jest.fn(); - const instance = { + const instance = createInstance({ revealLineInCenter: revealSpy, deltaDecorations: decorationsSpy, - }; + }); + instance.use({ definition: SourceEditorExtension }); const defaultDecorationOptions = { isWholeLine: true, className: 'active-line-text', @@ -175,7 +110,7 @@ describe('The basis for an Source Editor extension', () => { ${'uses bounds if both hash and bounds exist'} | ${'#L7-42'} | ${[3, 5]} | ${true} | ${[3, 1, 5, 1]} `('$desc', ({ hash, bounds, shouldReveal, expectedRange } = {}) => { window.location.hash = hash; - SourceEditorExtension.highlightLines(instance, bounds); + instance.highlightLines(bounds); if (!shouldReveal) { expect(revealSpy).not.toHaveBeenCalled(); expect(decorationsSpy).not.toHaveBeenCalled(); @@ -193,11 +128,11 @@ describe('The basis for an Source Editor extension', () => { } }); - it('stores the line decorations on the instance', () => { + it('stores the line decorations on the instance', () => { decorationsSpy.mockReturnValue('foo'); window.location.hash = '#L10'; expect(instance.lineDecorations).toBeUndefined(); - SourceEditorExtension.highlightLines(instance); + instance.highlightLines(); expect(instance.lineDecorations).toBe('foo'); }); @@ -215,7 +150,7 @@ describe('The basis for an Source Editor extension', () => { }, ]; instance.lineDecorations = oldLineDecorations; - SourceEditorExtension.highlightLines(instance, [7, 10]); + instance.highlightLines([7, 10]); expect(decorationsSpy).toHaveBeenCalledWith(oldLineDecorations, newLineDecorations); }); }); @@ -228,13 +163,18 @@ describe('The basis for an Source Editor extension', () => { options: { isWholeLine: true, className: 'active-line-text' }, }, ]; - const instance = { - deltaDecorations: decorationsSpy, - lineDecorations, - }; + let instance; + + beforeEach(() => { + instance = createInstance({ + deltaDecorations: decorationsSpy, + lineDecorations, + }); + instance.use({ definition: SourceEditorExtension }); + }); it('removes all existing decorations', () => { - SourceEditorExtension.removeHighlights(instance); + instance.removeHighlights(); expect(decorationsSpy).toHaveBeenCalledWith(lineDecorations, []); }); }); diff --git a/spec/frontend/editor/source_editor_spec.js b/spec/frontend/editor/source_editor_spec.js index a53eb6fc173655..11c0a1f969bb8e 100644 --- a/spec/frontend/editor/source_editor_spec.js +++ b/spec/frontend/editor/source_editor_spec.js @@ -8,6 +8,21 @@ import SourceEditor from '~/editor/source_editor'; import { DEFAULT_THEME, themes } from '~/ide/lib/themes'; import { joinPaths } from '~/lib/utils/url_utility'; +jest.mock('~/helpers/startup_css_helper', () => { + return { + waitForCSSLoaded: jest.fn().mockImplementation((cb) => { + // We have to artificially put the callback's execution + // to the end of the current call stack to be able to + // test that the callback is called after waitForCSSLoaded. + // setTimeout with 0 delay does exactly that. + // Otherwise we might end up with false positive results + setTimeout(() => { + cb.apply(); + }, 0); + }), + }; +}); + describe('Base editor', () => { let editorEl; let editor; @@ -142,6 +157,23 @@ describe('Base editor', () => { expect(editor.instances).toHaveLength(0); expect(instance.getModel()).toBeNull(); }); + + it('resets the layout in waitForCSSLoaded callback', async () => { + const layoutSpy = jest.fn(); + jest.spyOn(monacoEditor, 'create').mockReturnValue({ + layout: layoutSpy, + setModel: jest.fn(), + onDidDispose: jest.fn(), + dispose: jest.fn(), + }); + editor.createInstance(defaultArguments); + expect(layoutSpy).not.toHaveBeenCalled(); + + // We're waiting for the waitForCSSLoaded mock to kick in + await jest.runOnlyPendingTimers(); + + expect(layoutSpy).toHaveBeenCalled(); + }); }); describe('instance of the Diff Editor', () => { -- GitLab From 3bb1687574dd412e831de408a2a8a627604037d0 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Thu, 2 Dec 2021 10:27:12 +0100 Subject: [PATCH 04/25] Refactoring FileTemplate extension --- app/assets/javascripts/blob_edit/edit_blob.js | 3 ++- .../extensions/source_editor_file_template_ext.js | 12 ++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/blob_edit/edit_blob.js b/app/assets/javascripts/blob_edit/edit_blob.js index 118cef59d5a01b..4b2ffb110c79fe 100644 --- a/app/assets/javascripts/blob_edit/edit_blob.js +++ b/app/assets/javascripts/blob_edit/edit_blob.js @@ -1,4 +1,5 @@ import $ from 'jquery'; +import { SourceEditorExtension } from '~/editor/extensions/source_editor_extension_base'; import { FileTemplateExtension } from '~/editor/extensions/source_editor_file_template_ext'; import SourceEditor from '~/editor/source_editor'; import { getBlobLanguage } from '~/editor/utils'; @@ -60,7 +61,7 @@ export default class EditBlob { blobPath: fileNameEl.value, blobContent: editorEl.innerText, }); - this.editor.use(new FileTemplateExtension({ instance: this.editor })); + this.editor.use([{ definition: SourceEditorExtension }, { definition: FileTemplateExtension }]); fileNameEl.addEventListener('change', () => { this.editor.updateModelLanguage(fileNameEl.value); diff --git a/app/assets/javascripts/editor/extensions/source_editor_file_template_ext.js b/app/assets/javascripts/editor/extensions/source_editor_file_template_ext.js index 397e090ed30331..50b3ed54beed3c 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_file_template_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_file_template_ext.js @@ -1,8 +1,12 @@ import { Position } from 'monaco-editor'; -import { SourceEditorExtension } from './source_editor_extension_base'; -export class FileTemplateExtension extends SourceEditorExtension { - navigateFileStart() { - this.setPosition(new Position(1, 1)); +export class FileTemplateExtension { + // eslint-disable-next-line class-methods-use-this + provides() { + return { + navigateFileStart: (instance) => { + instance.setPosition(new Position(1, 1)); + }, + }; } } -- GitLab From f03ca963b89e60163e22b8fd7162cc1ebac2c737 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Tue, 7 Dec 2021 23:39:49 +0100 Subject: [PATCH 05/25] Moved use/unuse to explicit public API Instead of "hiding" use/use in the constructor, we make those explicitly defined in the public API of the module. In the same effort, we remove `set` interceptor from the proxy. It was needed exclusively for the tests, but after another look it didn't look to be necessary while breaking editability of the instance. --- .../editor/source_editor_instance.js | 36 ++++++++++--------- .../editor/source_editor_instance_spec.js | 6 ++-- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/app/assets/javascripts/editor/source_editor_instance.js b/app/assets/javascripts/editor/source_editor_instance.js index fcffdc587bed46..6e4fa77cc170f3 100644 --- a/app/assets/javascripts/editor/source_editor_instance.js +++ b/app/assets/javascripts/editor/source_editor_instance.js @@ -77,26 +77,10 @@ export default class EditorInstance { } return Reflect.get(seInstance[prop] ? seInstance : target, prop, receiver); }, - set(target, prop, value) { - Object.assign(seInstance, { - [prop]: value, - }); - return true; - }, }; const instProxy = new Proxy(rootInstance, getHandler); - /** - * Main entry point to apply an extension to the instance - * @param {SourceEditorExtensionDefinition} - */ - this.use = EditorInstance.useUnuse.bind(instProxy, extensionsStore, this.useExtension); - - /** - * Main entry point to un-use an extension and remove it from the instance - * @param {SourceEditorExtension} - */ - this.unuse = EditorInstance.useUnuse.bind(instProxy, extensionsStore, this.unuseExtension); + this.dispatchExtAction = EditorInstance.useUnuse.bind(instProxy, extensionsStore); return instProxy; } @@ -259,6 +243,24 @@ export default class EditorInstance { monacoEditor.setModelLanguage(model, lang); } + /** + * Main entry point to apply an extension to the instance + * @param {SourceEditorExtensionDefinition[]|SourceEditorExtensionDefinition} extDefs - The extension(s) to use + * @returns {EditorExtension|*} + */ + use(extDefs) { + return this.dispatchExtAction(this.useExtension, extDefs); + } + + /** + * Main entry point to remove an extension to the instance + * @param {SourceEditorExtension[]|SourceEditorExtension} exts - + * @returns {*} + */ + unuse(exts) { + return this.dispatchExtAction(this.unuseExtension, exts); + } + /** * Get the methods returned by extensions. * @returns {Array} diff --git a/spec/frontend/editor/source_editor_instance_spec.js b/spec/frontend/editor/source_editor_instance_spec.js index 38844b6cafeaea..8b8c0f311106b8 100644 --- a/spec/frontend/editor/source_editor_instance_spec.js +++ b/spec/frontend/editor/source_editor_instance_spec.js @@ -127,11 +127,11 @@ describe('Source Editor Instance', () => { seInstance = new SourceEditorInstance({ use: fooFn, }); - jest.spyOn(seInstance, 'use').mockImplementation(() => {}); - expect(seInstance.use).not.toHaveBeenCalled(); + const spy = jest.spyOn(seInstance.constructor.prototype, 'use').mockImplementation(() => {}); + expect(spy).not.toHaveBeenCalled(); expect(fooFn).not.toHaveBeenCalled(); seInstance.use(); - expect(seInstance.use).toHaveBeenCalled(); + expect(spy).toHaveBeenCalled(); expect(fooFn).not.toHaveBeenCalled(); }); -- GitLab From 362d2716cebb56c493b3b47753546eb006f23cd1 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Tue, 7 Dec 2021 23:57:10 +0100 Subject: [PATCH 06/25] Refactored the Markdown Extension --- .../extensions/source_editor_markdown_ext.js | 179 +++++++++--------- .../editor/source_editor_markdown_ext_spec.js | 5 +- 2 files changed, 91 insertions(+), 93 deletions(-) diff --git a/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js b/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js index bec7fe7e25fe3f..35f814c3949378 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js @@ -1,97 +1,98 @@ -import { EditorMarkdownPreviewExtension } from '~/editor/extensions/source_editor_markdown_livepreview_ext'; +export class EditorMarkdownExtension { + // eslint-disable-next-line class-methods-use-this + provides() { + return { + getSelectedText: (instance, selection = instance.getSelection()) => { + const { startLineNumber, endLineNumber, startColumn, endColumn } = selection; + const valArray = instance.getValue().split('\n'); + let text = ''; + if (startLineNumber === endLineNumber) { + text = valArray[startLineNumber - 1].slice(startColumn - 1, endColumn - 1); + } else { + const startLineText = valArray[startLineNumber - 1].slice(startColumn - 1); + const endLineText = valArray[endLineNumber - 1].slice(0, endColumn - 1); -export class EditorMarkdownExtension extends EditorMarkdownPreviewExtension { - getSelectedText(selection = this.getSelection()) { - const { startLineNumber, endLineNumber, startColumn, endColumn } = selection; - const valArray = this.getValue().split('\n'); - let text = ''; - if (startLineNumber === endLineNumber) { - text = valArray[startLineNumber - 1].slice(startColumn - 1, endColumn - 1); - } else { - const startLineText = valArray[startLineNumber - 1].slice(startColumn - 1); - const endLineText = valArray[endLineNumber - 1].slice(0, endColumn - 1); + for (let i = startLineNumber, k = endLineNumber - 1; i < k; i += 1) { + text += `${valArray[i]}`; + if (i !== k - 1) text += `\n`; + } + text = text + ? [startLineText, text, endLineText].join('\n') + : [startLineText, endLineText].join('\n'); + } + return text; + }, + replaceSelectedText: (instance, text, select = undefined) => { + const forceMoveMarkers = !select; + instance.executeEdits('', [{ range: instance.getSelection(), text, forceMoveMarkers }]); + }, + moveCursor: (instance, dx = 0, dy = 0) => { + const pos = instance.getPosition(); + pos.column += dx; + pos.lineNumber += dy; + instance.setPosition(pos); + }, + /** + * Adjust existing selection to select text within the original selection. + * - If `selectedText` is not supplied, we fetch selected text with + * + * ALGORITHM: + * + * MULTI-LINE SELECTION + * 1. Find line that contains `toSelect` text. + * 2. Using the index of this line and the position of `toSelect` text in it, + * construct: + * * newStartLineNumber + * * newStartColumn + * + * SINGLE-LINE SELECTION + * 1. Use `startLineNumber` from the current selection as `newStartLineNumber` + * 2. Find the position of `toSelect` text in it to get `newStartColumn` + * + * 3. `newEndLineNumber` — Since this method is supposed to be used with + * markdown decorators that are pretty short, the `newEndLineNumber` is + * suggested to be assumed the same as the startLine. + * 4. `newEndColumn` — pretty obvious + * 5. Adjust the start and end positions of the current selection + * 6. Re-set selection on the instance + * + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance. Is passed automatically. + * @param {string} toSelect - New text to select within current selection. + * @param {string} selectedText - Currently selected text. It's just a + * shortcut: If it's not supplied, we fetch selected text from the instance + */ + selectWithinSelection: (instance, toSelect, selectedText) => { + const currentSelection = instance.getSelection(); + if (currentSelection.isEmpty() || !toSelect) { + return; + } + const text = selectedText || instance.getSelectedText(currentSelection); + let lineShift; + let newStartLineNumber; + let newStartColumn; - for (let i = startLineNumber, k = endLineNumber - 1; i < k; i += 1) { - text += `${valArray[i]}`; - if (i !== k - 1) text += `\n`; - } - text = text - ? [startLineText, text, endLineText].join('\n') - : [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. - * - If `selectedText` is not supplied, we fetch selected text with - * - * ALGORITHM: - * - * MULTI-LINE SELECTION - * 1. Find line that contains `toSelect` text. - * 2. Using the index of this line and the position of `toSelect` text in it, - * construct: - * * newStartLineNumber - * * newStartColumn - * - * SINGLE-LINE SELECTION - * 1. Use `startLineNumber` from the current selection as `newStartLineNumber` - * 2. Find the position of `toSelect` text in it to get `newStartColumn` - * - * 3. `newEndLineNumber` — Since this method is supposed to be used with - * markdown decorators that are pretty short, the `newEndLineNumber` is - * suggested to be assumed the same as the startLine. - * 4. `newEndColumn` — pretty obvious - * 5. Adjust the start and end positions of the current selection - * 6. Re-set selection on the instance - * - * @param {string} toSelect - New text to select within current selection. - * @param {string} selectedText - Currently selected text. It's just a - * shortcut: If it's not supplied, we fetch selected text from the instance - */ - selectWithinSelection(toSelect, selectedText) { - const currentSelection = this.getSelection(); - if (currentSelection.isEmpty() || !toSelect) { - return; - } - const text = selectedText || this.getSelectedText(currentSelection); - let lineShift; - let newStartLineNumber; - let newStartColumn; - - const textLines = text.split('\n'); + const textLines = text.split('\n'); - if (textLines.length > 1) { - // Multi-line selection - lineShift = textLines.findIndex((line) => line.indexOf(toSelect) !== -1); - newStartLineNumber = currentSelection.startLineNumber + lineShift; - newStartColumn = textLines[lineShift].indexOf(toSelect) + 1; - } else { - // Single-line selection - newStartLineNumber = currentSelection.startLineNumber; - newStartColumn = currentSelection.startColumn + text.indexOf(toSelect); - } + if (textLines.length > 1) { + // Multi-line selection + lineShift = textLines.findIndex((line) => line.indexOf(toSelect) !== -1); + newStartLineNumber = currentSelection.startLineNumber + lineShift; + newStartColumn = textLines[lineShift].indexOf(toSelect) + 1; + } else { + // Single-line selection + newStartLineNumber = currentSelection.startLineNumber; + newStartColumn = currentSelection.startColumn + text.indexOf(toSelect); + } - const newEndLineNumber = newStartLineNumber; - const newEndColumn = newStartColumn + toSelect.length; + const newEndLineNumber = newStartLineNumber; + const newEndColumn = newStartColumn + toSelect.length; - const newSelection = currentSelection - .setStartPosition(newStartLineNumber, newStartColumn) - .setEndPosition(newEndLineNumber, newEndColumn); + const newSelection = currentSelection + .setStartPosition(newStartLineNumber, newStartColumn) + .setEndPosition(newEndLineNumber, newEndColumn); - this.setSelection(newSelection); + instance.setSelection(newSelection); + }, + }; } } diff --git a/spec/frontend/editor/source_editor_markdown_ext_spec.js b/spec/frontend/editor/source_editor_markdown_ext_spec.js index 4a50d801296021..eecd23bff6e773 100644 --- a/spec/frontend/editor/source_editor_markdown_ext_spec.js +++ b/spec/frontend/editor/source_editor_markdown_ext_spec.js @@ -9,7 +9,6 @@ describe('Markdown Extension for Source Editor', () => { let instance; let editorEl; let mockAxios; - const previewMarkdownPath = '/gitlab/fooGroup/barProj/preview_markdown'; const firstLine = 'This is a'; const secondLine = 'multiline'; const thirdLine = 'string with some **markup**'; @@ -36,7 +35,7 @@ describe('Markdown Extension for Source Editor', () => { blobPath: markdownPath, blobContent: text, }); - instance.use(new EditorMarkdownExtension({ instance, previewMarkdownPath })); + instance.use({ definition: EditorMarkdownExtension }); }); afterEach(() => { @@ -164,13 +163,11 @@ describe('Markdown Extension for Source Editor', () => { }); it('does not fail when only `toSelect` is supplied and fetches the text from selection', () => { - jest.spyOn(instance, 'getSelectedText'); const toSelect = 'string'; selectSecondAndThirdLines(); instance.selectWithinSelection(toSelect); - expect(instance.getSelectedText).toHaveBeenCalled(); expect(selectionToString()).toBe(`[3,1 -> 3,${toSelect.length + 1}]`); }); -- GitLab From 895e66dbf46c4a86445b4738dd572713ca549e05 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Wed, 8 Dec 2021 22:08:30 +0100 Subject: [PATCH 07/25] Pass correct context for extension's methods Now we set context to be the extension object instead of the instance which is passed in as a n explicit parameter --- .../editor/source_editor_instance.js | 2 +- spec/frontend/editor/helpers.js | 49 ++++++++++--------- .../editor/source_editor_instance_spec.js | 4 +- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/app/assets/javascripts/editor/source_editor_instance.js b/app/assets/javascripts/editor/source_editor_instance.js index 6e4fa77cc170f3..faf009629dd4bd 100644 --- a/app/assets/javascripts/editor/source_editor_instance.js +++ b/app/assets/javascripts/editor/source_editor_instance.js @@ -73,7 +73,7 @@ export default class EditorInstance { if (methodExtension) { const extension = extensionsStore.get(methodExtension); - return (...args) => extension.api[prop].call(seInstance, receiver, ...args); + return extension.api[prop].bind(extension.obj, receiver); } return Reflect.get(seInstance[prop] ? seInstance : target, prop, receiver); }, diff --git a/spec/frontend/editor/helpers.js b/spec/frontend/editor/helpers.js index e4942c36f6c3fe..6663dfcf37df92 100644 --- a/spec/frontend/editor/helpers.js +++ b/spec/frontend/editor/helpers.js @@ -1,3 +1,5 @@ +/* eslint-disable max-classes-per-file */ + export class SEClassExtension { // eslint-disable-next-line class-methods-use-this provides() { @@ -29,31 +31,30 @@ export const SEConstExt = () => { }; }; -export function SEWithSetupExt() { - return { - onSetup: (instance, setupOptions = {}) => { - if (setupOptions && !Array.isArray(setupOptions)) { - Object.entries(setupOptions).forEach(([key, value]) => { - Object.assign(instance, { - [key]: value, - }); +export class SEWithSetupExt { + // eslint-disable-next-line class-methods-use-this + onSetup(instance, setupOptions = {}) { + if (setupOptions && !Array.isArray(setupOptions)) { + Object.entries(setupOptions).forEach(([key, value]) => { + Object.assign(instance, { + [key]: value, }); - } - }, - provides: () => { - return { - returnInstanceAndProps: (instance, stringProp, objProp = {}) => { - return [stringProp, objProp, instance]; - }, - returnInstance: (instance) => { - return instance; - }, - giveMeContext: () => { - return this; - }, - }; - }, - }; + }); + } + } + provides() { + return { + returnInstanceAndProps: (instance, stringProp, objProp = {}) => { + return [stringProp, objProp, instance]; + }, + returnInstance: (instance) => { + return instance; + }, + giveMeContext: () => { + return this; + }, + }; + } } export const conflictingExtensions = { diff --git a/spec/frontend/editor/source_editor_instance_spec.js b/spec/frontend/editor/source_editor_instance_spec.js index 8b8c0f311106b8..4fd84186a87398 100644 --- a/spec/frontend/editor/source_editor_instance_spec.js +++ b/spec/frontend/editor/source_editor_instance_spec.js @@ -118,9 +118,9 @@ describe('Source Editor Instance', () => { it("correctly sets the context of the 'this' keyword for the extension's methods", () => { seInstance = new SourceEditorInstance(); - seInstance.use({ definition: SEWithSetupExt }); + const extension = seInstance.use({ definition: SEWithSetupExt }); - expect(seInstance.giveMeContext().constructor).toEqual(SEWithSetupExt); + expect(seInstance.giveMeContext()).toEqual(extension.obj); }); it('returns props from SE instance itself if no extension provides the prop', () => { -- GitLab From 8275dcf3e6cead2f01ee0aad31b0c17fbd4d942b Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Wed, 8 Dec 2021 23:57:27 +0100 Subject: [PATCH 08/25] Allow any properties in extension's public API --- .../javascripts/editor/source_editor_instance.js | 6 +++++- spec/frontend/editor/source_editor_instance_spec.js | 11 ++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/editor/source_editor_instance.js b/app/assets/javascripts/editor/source_editor_instance.js index faf009629dd4bd..c307a982b82cfd 100644 --- a/app/assets/javascripts/editor/source_editor_instance.js +++ b/app/assets/javascripts/editor/source_editor_instance.js @@ -73,7 +73,11 @@ export default class EditorInstance { if (methodExtension) { const extension = extensionsStore.get(methodExtension); - return extension.api[prop].bind(extension.obj, receiver); + if (typeof extension.api[prop] === 'function') { + return extension.api[prop].bind(extension.obj, receiver); + } + + return extension.api[prop]; } return Reflect.get(seInstance[prop] ? seInstance : target, prop, receiver); }, diff --git a/spec/frontend/editor/source_editor_instance_spec.js b/spec/frontend/editor/source_editor_instance_spec.js index 4fd84186a87398..f13c88346ba984 100644 --- a/spec/frontend/editor/source_editor_instance_spec.js +++ b/spec/frontend/editor/source_editor_instance_spec.js @@ -32,11 +32,13 @@ describe('Source Editor Instance', () => { ]; const fooFn = jest.fn(); + const fooProp = 'foo'; class DummyExt { // eslint-disable-next-line class-methods-use-this provides() { return { fooFn, + fooProp, }; } } @@ -64,7 +66,7 @@ describe('Source Editor Instance', () => { }); describe('proxy', () => { - it('returns prop from an extension if extension provides it', () => { + it('returns a method from an extension if extension provides it', () => { seInstance = new SourceEditorInstance(); seInstance.use({ definition: DummyExt }); @@ -73,6 +75,13 @@ describe('Source Editor Instance', () => { expect(fooFn).toHaveBeenCalled(); }); + it('returns a prop from an extension if extension provides it', () => { + seInstance = new SourceEditorInstance(); + seInstance.use({ definition: DummyExt }); + + expect(seInstance.fooProp).toBe('foo'); + }); + it.each` stringPropToPass | objPropToPass | setupOptions ${undefined} | ${undefined} | ${undefined} -- GitLab From 5257335fd3662e05b9f4beb6f3ad0e08a631b0b1 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Thu, 9 Dec 2021 00:57:48 +0100 Subject: [PATCH 09/25] The new spyOnApi helper method to work with SE extensions --- spec/frontend/editor/helpers.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/spec/frontend/editor/helpers.js b/spec/frontend/editor/helpers.js index 6663dfcf37df92..dfdbf6913e576c 100644 --- a/spec/frontend/editor/helpers.js +++ b/spec/frontend/editor/helpers.js @@ -1,5 +1,19 @@ /* eslint-disable max-classes-per-file */ +// Helpers +export const spyOnApi = (extension, spiesObj = {}) => { + const origApi = extension.api; + if (extension?.obj) { + jest.spyOn(extension.obj, 'provides').mockImplementation(() => { + return { + ...origApi, + ...spiesObj, + }; + }); + } +}; + +// Dummy Extensions export class SEClassExtension { // eslint-disable-next-line class-methods-use-this provides() { -- GitLab From 59e02facc2adce2509b8248476c389b99f5ad74a Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Thu, 9 Dec 2021 01:14:26 +0100 Subject: [PATCH 10/25] Refactor the Markdown Live Preview extension --- .../source_editor_markdown_livepreview_ext.js | 117 ++++++++++-------- ...ce_editor_markdown_livepreview_ext_spec.js | 103 +++++++++------ 2 files changed, 126 insertions(+), 94 deletions(-) diff --git a/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js b/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js index 526de7f89320f1..921a6028b95663 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js @@ -12,9 +12,8 @@ import { EXTENSION_MARKDOWN_PREVIEW_PANEL_PARENT_CLASS, EXTENSION_MARKDOWN_PREVIEW_UPDATE_DELAY, } from '../constants'; -import { SourceEditorExtension } from './source_editor_extension_base'; -const getPreview = (text, previewMarkdownPath) => { +const fetchtPreview = (text, previewMarkdownPath) => { return axios .post(previewMarkdownPath, { text, @@ -34,19 +33,16 @@ const setupDomElement = ({ injectToEl = null } = {}) => { return previewEl; }; -export class EditorMarkdownPreviewExtension extends SourceEditorExtension { - constructor({ instance, previewMarkdownPath, ...args } = {}) { - super({ instance, ...args }); - Object.assign(instance, { - previewMarkdownPath, - preview: { - el: undefined, - action: undefined, - shown: false, - modelChangeListener: undefined, - }, - }); - this.setupPreviewAction.call(instance); +export class EditorMarkdownPreviewExtension { + onSetup(instance, setupOptions) { + this.preview = { + el: undefined, + action: undefined, + shown: false, + modelChangeListener: undefined, + path: setupOptions.previewMarkdownPath, + }; + this.setupPreviewAction(instance); instance.getModel().onDidChangeLanguage(({ newLanguage, oldLanguage } = {}) => { if (newLanguage === 'markdown' && oldLanguage !== newLanguage) { @@ -68,43 +64,31 @@ export class EditorMarkdownPreviewExtension extends SourceEditorExtension { }); } - static togglePreviewLayout() { - const { width, height } = this.getLayoutInfo(); + togglePreviewLayout(instance) { + const { width, height } = instance.getLayoutInfo(); const newWidth = this.preview.shown ? width / EXTENSION_MARKDOWN_PREVIEW_PANEL_WIDTH : width * EXTENSION_MARKDOWN_PREVIEW_PANEL_WIDTH; - this.layout({ width: newWidth, height }); + instance.layout({ width: newWidth, height }); } - static togglePreviewPanel() { - const parentEl = this.getDomNode().parentElement; + togglePreviewPanel(instance) { + const parentEl = instance.getDomNode().parentElement; const { el: previewEl } = this.preview; parentEl.classList.toggle(EXTENSION_MARKDOWN_PREVIEW_PANEL_PARENT_CLASS); if (previewEl.style.display === 'none') { // Show the preview panel - this.fetchPreview(); + this.fetchPreview(instance); } else { // Hide the preview panel previewEl.style.display = 'none'; } } - cleanup() { - if (this.preview.modelChangeListener) { - this.preview.modelChangeListener.dispose(); - } - this.preview.action.dispose(); - if (this.preview.shown) { - EditorMarkdownPreviewExtension.togglePreviewPanel.call(this); - EditorMarkdownPreviewExtension.togglePreviewLayout.call(this); - } - this.preview.shown = false; - } - - fetchPreview() { + fetchPreview(instance) { const { el: previewEl } = this.preview; - getPreview(this.getValue(), this.previewMarkdownPath) + fetchtPreview(instance.getValue(), this.preview.path) .then((data) => { previewEl.innerHTML = sanitize(data); syntaxHighlight(previewEl.querySelectorAll('.js-syntax-highlight')); @@ -113,10 +97,10 @@ export class EditorMarkdownPreviewExtension extends SourceEditorExtension { .catch(() => createFlash(BLOB_PREVIEW_ERROR)); } - setupPreviewAction() { - if (this.getAction(EXTENSION_MARKDOWN_PREVIEW_ACTION_ID)) return; + setupPreviewAction(instance) { + if (instance.getAction(EXTENSION_MARKDOWN_PREVIEW_ACTION_ID)) return; - this.preview.action = this.addAction({ + this.preview.action = instance.addAction({ id: EXTENSION_MARKDOWN_PREVIEW_ACTION_ID, label: __('Preview Markdown'), keybindings: [ @@ -128,27 +112,52 @@ export class EditorMarkdownPreviewExtension extends SourceEditorExtension { // Method that will be executed when the action is triggered. // @param ed The editor instance is passed in as a convenience - run(instance) { - instance.togglePreview(); + run(inst) { + inst.togglePreview(); }, }); } - togglePreview() { - if (!this.preview?.el) { - this.preview.el = setupDomElement({ injectToEl: this.getDomNode().parentElement }); - } - EditorMarkdownPreviewExtension.togglePreviewLayout.call(this); - EditorMarkdownPreviewExtension.togglePreviewPanel.call(this); + provides() { + return { + markdownPreview: this.preview, - if (!this.preview?.shown) { - this.preview.modelChangeListener = this.onDidChangeModelContent( - debounce(this.fetchPreview.bind(this), EXTENSION_MARKDOWN_PREVIEW_UPDATE_DELAY), - ); - } else { - this.preview.modelChangeListener.dispose(); - } + cleanup: (instance) => { + if (this.preview.modelChangeListener) { + this.preview.modelChangeListener.dispose(); + } + this.preview.action.dispose(); + if (this.preview.shown) { + this.togglePreviewPanel(instance); + this.togglePreviewLayout(instance); + } + this.preview.shown = false; + }, + + fetchPreview: (instance) => this.fetchPreview(instance), + + setupPreviewAction: (instance) => this.setupPreviewAction(instance), - this.preview.shown = !this.preview?.shown; + togglePreview: (instance) => { + if (!this.preview?.el) { + this.preview.el = setupDomElement({ injectToEl: instance.getDomNode().parentElement }); + } + this.togglePreviewLayout(instance); + this.togglePreviewPanel(instance); + + if (!this.preview?.shown) { + this.preview.modelChangeListener = instance.onDidChangeModelContent( + debounce( + this.fetchPreview.bind(this, instance), + EXTENSION_MARKDOWN_PREVIEW_UPDATE_DELAY, + ), + ); + } else { + this.preview.modelChangeListener.dispose(); + } + + this.preview.shown = !this.preview?.shown; + }, + }; } } diff --git a/spec/frontend/editor/source_editor_markdown_livepreview_ext_spec.js b/spec/frontend/editor/source_editor_markdown_livepreview_ext_spec.js index 3d797073c05d64..c8d016e10ac887 100644 --- a/spec/frontend/editor/source_editor_markdown_livepreview_ext_spec.js +++ b/spec/frontend/editor/source_editor_markdown_livepreview_ext_spec.js @@ -13,6 +13,7 @@ import SourceEditor from '~/editor/source_editor'; import createFlash from '~/flash'; import axios from '~/lib/utils/axios_utils'; import syntaxHighlight from '~/syntax_highlight'; +import { spyOnApi } from './helpers'; jest.mock('~/syntax_highlight'); jest.mock('~/flash'); @@ -23,6 +24,7 @@ describe('Markdown Live Preview Extension for Source Editor', () => { let editorEl; let panelSpy; let mockAxios; + let extension; const previewMarkdownPath = '/gitlab/fooGroup/barProj/preview_markdown'; const firstLine = 'This is a'; const secondLine = 'multiline'; @@ -47,8 +49,11 @@ describe('Markdown Live Preview Extension for Source Editor', () => { blobPath: markdownPath, blobContent: text, }); - instance.use(new EditorMarkdownPreviewExtension({ instance, previewMarkdownPath })); - panelSpy = jest.spyOn(EditorMarkdownPreviewExtension, 'togglePreviewPanel'); + extension = instance.use({ + definition: EditorMarkdownPreviewExtension, + setupOptions: { previewMarkdownPath }, + }); + panelSpy = jest.spyOn(extension.obj.constructor.prototype, 'togglePreviewPanel'); }); afterEach(() => { @@ -57,14 +62,14 @@ describe('Markdown Live Preview Extension for Source Editor', () => { mockAxios.restore(); }); - it('sets up the instance', () => { - expect(instance.preview).toEqual({ + it('sets up the preview on the instance', () => { + expect(instance.markdownPreview).toEqual({ el: undefined, action: expect.any(Object), shown: false, modelChangeListener: undefined, + path: previewMarkdownPath, }); - expect(instance.previewMarkdownPath).toBe(previewMarkdownPath); }); describe('model language changes listener', () => { @@ -72,14 +77,22 @@ describe('Markdown Live Preview Extension for Source Editor', () => { let actionSpy; beforeEach(async () => { - cleanupSpy = jest.spyOn(instance, 'cleanup'); - actionSpy = jest.spyOn(instance, 'setupPreviewAction'); + cleanupSpy = jest.fn(); + actionSpy = jest.fn(); + spyOnApi(extension, { + cleanup: cleanupSpy, + setupPreviewAction: actionSpy, + }); await togglePreview(); }); + afterEach(() => { + jest.clearAllMocks(); + }); + it('cleans up when switching away from markdown', () => { - expect(instance.cleanup).not.toHaveBeenCalled(); - expect(instance.setupPreviewAction).not.toHaveBeenCalled(); + expect(cleanupSpy).not.toHaveBeenCalled(); + expect(actionSpy).not.toHaveBeenCalled(); instance.updateModelLanguage(plaintextPath); @@ -110,8 +123,12 @@ describe('Markdown Live Preview Extension for Source Editor', () => { let actionSpy; beforeEach(() => { - cleanupSpy = jest.spyOn(instance, 'cleanup'); - actionSpy = jest.spyOn(instance, 'setupPreviewAction'); + cleanupSpy = jest.fn(); + actionSpy = jest.fn(); + spyOnApi(extension, { + cleanup: cleanupSpy, + setupPreviewAction: actionSpy, + }); instance.togglePreview(); }); @@ -153,14 +170,17 @@ describe('Markdown Live Preview Extension for Source Editor', () => { }); it('disposes the modelChange listener and does not fetch preview on content changes', () => { - expect(instance.preview.modelChangeListener).toBeDefined(); - jest.spyOn(instance, 'fetchPreview'); + expect(instance.markdownPreview.modelChangeListener).toBeDefined(); + const fetchPreviewSpy = jest.fn(); + spyOnApi(extension, { + fetchPreview: fetchPreviewSpy, + }); instance.cleanup(); instance.setValue('Foo Bar'); jest.advanceTimersByTime(EXTENSION_MARKDOWN_PREVIEW_UPDATE_DELAY); - expect(instance.fetchPreview).not.toHaveBeenCalled(); + expect(fetchPreviewSpy).not.toHaveBeenCalled(); }); it('removes the contextual menu action', () => { @@ -172,13 +192,13 @@ describe('Markdown Live Preview Extension for Source Editor', () => { }); it('toggles the `shown` flag', () => { - expect(instance.preview.shown).toBe(true); + expect(instance.markdownPreview.shown).toBe(true); instance.cleanup(); - expect(instance.preview.shown).toBe(false); + expect(instance.markdownPreview.shown).toBe(false); }); it('toggles the panel only if the preview is visible', () => { - const { el: previewEl } = instance.preview; + const { el: previewEl } = instance.markdownPreview; const parentEl = previewEl.parentElement; expect(previewEl).toBeVisible(); @@ -200,7 +220,7 @@ describe('Markdown Live Preview Extension for Source Editor', () => { it('toggles the layout only if the preview is visible', () => { const { width } = instance.getLayoutInfo(); - expect(instance.preview.shown).toBe(true); + expect(instance.markdownPreview.shown).toBe(true); instance.cleanup(); @@ -234,13 +254,13 @@ describe('Markdown Live Preview Extension for Source Editor', () => { }); it('puts the fetched content into the preview DOM element', async () => { - instance.preview.el = editorEl.parentElement; + instance.markdownPreview.el = editorEl.parentElement; await fetchPreview(); - expect(instance.preview.el.innerHTML).toEqual(responseData); + expect(instance.markdownPreview.el.innerHTML).toEqual(responseData); }); it('applies syntax highlighting to the preview content', async () => { - instance.preview.el = editorEl.parentElement; + instance.markdownPreview.el = editorEl.parentElement; await fetchPreview(); expect(syntaxHighlight).toHaveBeenCalled(); }); @@ -266,14 +286,17 @@ describe('Markdown Live Preview Extension for Source Editor', () => { }); it('toggles preview when the action is triggered', () => { - jest.spyOn(instance, 'togglePreview').mockImplementation(); + const togglePreviewSpy = jest.fn(); + spyOnApi(extension, { + togglePreview: togglePreviewSpy, + }); - expect(instance.togglePreview).not.toHaveBeenCalled(); + expect(togglePreviewSpy).not.toHaveBeenCalled(); const action = instance.getAction(EXTENSION_MARKDOWN_PREVIEW_ACTION_ID); action.run(); - expect(instance.togglePreview).toHaveBeenCalled(); + expect(togglePreviewSpy).toHaveBeenCalled(); }); }); @@ -283,39 +306,39 @@ describe('Markdown Live Preview Extension for Source Editor', () => { }); it('toggles preview flag on instance', () => { - expect(instance.preview.shown).toBe(false); + expect(instance.markdownPreview.shown).toBe(false); instance.togglePreview(); - expect(instance.preview.shown).toBe(true); + expect(instance.markdownPreview.shown).toBe(true); instance.togglePreview(); - expect(instance.preview.shown).toBe(false); + expect(instance.markdownPreview.shown).toBe(false); }); describe('panel DOM element set up', () => { it('sets up an element to contain the preview and stores it on instance', () => { - expect(instance.preview.el).toBeUndefined(); + expect(instance.markdownPreview.el).toBeUndefined(); instance.togglePreview(); - expect(instance.preview.el).toBeDefined(); - expect(instance.preview.el.classList.contains(EXTENSION_MARKDOWN_PREVIEW_PANEL_CLASS)).toBe( - true, - ); + expect(instance.markdownPreview.el).toBeDefined(); + expect( + instance.markdownPreview.el.classList.contains(EXTENSION_MARKDOWN_PREVIEW_PANEL_CLASS), + ).toBe(true); }); it('re-uses existing preview DOM element on repeated calls', () => { instance.togglePreview(); - const origPreviewEl = instance.preview.el; + const origPreviewEl = instance.markdownPreview.el; instance.togglePreview(); - expect(instance.preview.el).toBe(origPreviewEl); + expect(instance.markdownPreview.el).toBe(origPreviewEl); }); it('hides the preview DOM element by default', () => { panelSpy.mockImplementation(); instance.togglePreview(); - expect(instance.preview.el.style.display).toBe('none'); + expect(instance.markdownPreview.el.style.display).toBe('none'); }); }); @@ -350,9 +373,9 @@ describe('Markdown Live Preview Extension for Source Editor', () => { it('toggles visibility of the preview DOM element', async () => { await togglePreview(); - expect(instance.preview.el.style.display).toBe('block'); + expect(instance.markdownPreview.el.style.display).toBe('block'); await togglePreview(); - expect(instance.preview.el.style.display).toBe('none'); + expect(instance.markdownPreview.el.style.display).toBe('none'); }); describe('hidden preview DOM element', () => { @@ -367,9 +390,9 @@ describe('Markdown Live Preview Extension for Source Editor', () => { }); it('stores disposable listener for model changes', async () => { - expect(instance.preview.modelChangeListener).toBeUndefined(); + expect(instance.markdownPreview.modelChangeListener).toBeUndefined(); await togglePreview(); - expect(instance.preview.modelChangeListener).toBeDefined(); + expect(instance.markdownPreview.modelChangeListener).toBeDefined(); }); }); @@ -386,7 +409,7 @@ describe('Markdown Live Preview Extension for Source Editor', () => { it('disposes the model change event listener', () => { const disposeSpy = jest.fn(); - instance.preview.modelChangeListener = { + instance.markdownPreview.modelChangeListener = { dispose: disposeSpy, }; instance.togglePreview(); -- GitLab From 386853af9aee4b9559f396936f1791d91aa48e8e Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Thu, 9 Dec 2021 01:44:06 +0100 Subject: [PATCH 11/25] Updated use of extensions in edit_blob --- app/assets/javascripts/blob_edit/edit_blob.js | 31 ++++++++++------ spec/frontend/blob_edit/edit_blob_spec.js | 35 +++++++++++++------ 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/blob_edit/edit_blob.js b/app/assets/javascripts/blob_edit/edit_blob.js index 4b2ffb110c79fe..43a0bbef948406 100644 --- a/app/assets/javascripts/blob_edit/edit_blob.js +++ b/app/assets/javascripts/blob_edit/edit_blob.js @@ -28,17 +28,26 @@ export default class EditBlob { } fetchMarkdownExtension() { - import('~/editor/extensions/source_editor_markdown_ext') - .then(({ EditorMarkdownExtension: MarkdownExtension } = {}) => { - this.editor.use( - new MarkdownExtension({ - instance: this.editor, - previewMarkdownPath: this.options.previewMarkdownPath, - }), - ); - this.hasMarkdownExtension = true; - addEditorMarkdownListeners(this.editor); - }) + return Promise.all([ + import('~/editor/extensions/source_editor_markdown_ext'), + import('~/editor/extensions/source_editor_markdown_livepreview_ext'), + ]) + .then( + ([ + { EditorMarkdownExtension: MarkdownExtension }, + { EditorMarkdownPreviewExtension: MarkdownLivePreview }, + ]) => { + this.editor.use([ + { definition: MarkdownExtension }, + { + definition: MarkdownLivePreview, + setupOptions: { previewMarkdownPath: this.options.previewMarkdownPath }, + }, + ]); + this.hasMarkdownExtension = true; + addEditorMarkdownListeners(this.editor); + }, + ) .catch((e) => createFlash({ message: `${BLOB_EDITOR_ERROR}: ${e}`, diff --git a/spec/frontend/blob_edit/edit_blob_spec.js b/spec/frontend/blob_edit/edit_blob_spec.js index ebef065675007c..9c974e79e6e3a4 100644 --- a/spec/frontend/blob_edit/edit_blob_spec.js +++ b/spec/frontend/blob_edit/edit_blob_spec.js @@ -1,14 +1,29 @@ import waitForPromises from 'helpers/wait_for_promises'; import EditBlob from '~/blob_edit/edit_blob'; +import { SourceEditorExtension } from '~/editor/extensions/source_editor_extension_base'; import { FileTemplateExtension } from '~/editor/extensions/source_editor_file_template_ext'; import { EditorMarkdownExtension } from '~/editor/extensions/source_editor_markdown_ext'; +import { EditorMarkdownPreviewExtension } from '~/editor/extensions/source_editor_markdown_livepreview_ext'; import SourceEditor from '~/editor/source_editor'; jest.mock('~/editor/source_editor'); -jest.mock('~/editor/extensions/source_editor_markdown_ext'); +jest.mock('~/editor/extensions/source_editor_extension_base'); jest.mock('~/editor/extensions/source_editor_file_template_ext'); +jest.mock('~/editor/extensions/source_editor_markdown_ext'); +jest.mock('~/editor/extensions/source_editor_markdown_livepreview_ext'); const PREVIEW_MARKDOWN_PATH = '/foo/bar/preview_markdown'; +const defaultExtensions = [ + { definition: SourceEditorExtension }, + { definition: FileTemplateExtension }, +]; +const markdownExtensions = [ + { definition: EditorMarkdownExtension }, + { + definition: EditorMarkdownPreviewExtension, + setupOptions: { previewMarkdownPath: PREVIEW_MARKDOWN_PATH }, + }, +]; describe('Blob Editing', () => { const useMock = jest.fn(); @@ -29,7 +44,9 @@ describe('Blob Editing', () => { jest.spyOn(SourceEditor.prototype, 'createInstance').mockReturnValue(mockInstance); }); afterEach(() => { + SourceEditorExtension.mockClear(); EditorMarkdownExtension.mockClear(); + EditorMarkdownPreviewExtension.mockClear(); FileTemplateExtension.mockClear(); }); @@ -45,26 +62,22 @@ describe('Blob Editing', () => { await waitForPromises(); }; - it('loads FileTemplateExtension by default', async () => { + it('loads SourceEditorExtension and FileTemplateExtension by default', async () => { await initEditor(); - expect(useMock).toHaveBeenCalledWith(expect.any(FileTemplateExtension)); - expect(FileTemplateExtension).toHaveBeenCalledTimes(1); + expect(useMock).toHaveBeenCalledWith(defaultExtensions); }); describe('Markdown', () => { - it('does not load MarkdownExtension by default', async () => { + it('does not load MarkdownExtensions by default', async () => { await initEditor(); expect(EditorMarkdownExtension).not.toHaveBeenCalled(); + expect(EditorMarkdownPreviewExtension).not.toHaveBeenCalled(); }); it('loads MarkdownExtension only for the markdown files', async () => { await initEditor(true); - expect(useMock).toHaveBeenCalledWith(expect.any(EditorMarkdownExtension)); - expect(EditorMarkdownExtension).toHaveBeenCalledTimes(1); - expect(EditorMarkdownExtension).toHaveBeenCalledWith({ - instance: mockInstance, - previewMarkdownPath: PREVIEW_MARKDOWN_PATH, - }); + expect(useMock).toHaveBeenCalledTimes(2); + expect(useMock.mock.calls[1]).toEqual([markdownExtensions]); }); }); -- GitLab From 86c7261be58b462d8776c10ca7873110a43d242c Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Thu, 9 Dec 2021 16:42:54 +0100 Subject: [PATCH 12/25] Refactored WebIDE extension --- .../extensions/source_editor_webide_ext.js | 272 ++++++++++-------- .../ide/components/repo_editor.vue | 39 +-- .../ide/components/repo_editor_spec.js | 100 ++++--- 3 files changed, 220 insertions(+), 191 deletions(-) diff --git a/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js b/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js index 98e05489c1c332..ba622bef20ebc7 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js @@ -1,7 +1,15 @@ +/** + * A WebIDE Extension options for Source Editor + * @typedef {Object} WebIDEExtensionOptions + * @property {Object} modelManager The root manager for WebIDE models + * @property {Object} store The state store for communication + * @property {Object} file + * @property {Object} options The Monaco editor options + */ + import { debounce } from 'lodash'; import { KeyCode, KeyMod, Range } from 'monaco-editor'; import { EDITOR_TYPE_DIFF } from '~/editor/constants'; -import { SourceEditorExtension } from '~/editor/extensions/source_editor_extension_base'; import Disposable from '~/ide/lib/common/disposable'; import { editorOptions } from '~/ide/lib/editor_options'; import keymap from '~/ide/lib/keymap.json'; @@ -11,154 +19,164 @@ const isDiffEditorType = (instance) => { }; export const UPDATE_DIMENSIONS_DELAY = 200; +const defaultOptions = { + modelManager: undefined, + store: undefined, + file: undefined, + options: {}, +}; -export class EditorWebIdeExtension extends SourceEditorExtension { - constructor({ instance, modelManager, ...options } = {}) { - super({ - instance, - ...options, - modelManager, - disposable: new Disposable(), - debouncedUpdate: debounce(() => { - instance.updateDimensions(); - }, UPDATE_DIMENSIONS_DELAY), - }); +const addActions = (instance, store) => { + const getKeyCode = (key) => { + const monacoKeyMod = key.indexOf('KEY_') === 0; - window.addEventListener('resize', instance.debouncedUpdate, false); + return monacoKeyMod ? KeyCode[key] : KeyMod[key]; + }; - instance.onDidDispose(() => { - window.removeEventListener('resize', instance.debouncedUpdate); - - // catch any potential errors with disposing the error - // this is mainly for tests caused by elements not existing - try { - instance.disposable.dispose(); - } catch (e) { - if (process.env.NODE_ENV !== 'test') { - // eslint-disable-next-line no-console - console.error(e); - } - } - }); - - EditorWebIdeExtension.addActions(instance); - } + keymap.forEach((command) => { + const { bindings, id, label, action } = command; - static addActions(instance) { - const { store } = instance; - const getKeyCode = (key) => { - const monacoKeyMod = key.indexOf('KEY_') === 0; - - return monacoKeyMod ? KeyCode[key] : KeyMod[key]; - }; + const keybindings = bindings.map((binding) => { + const keys = binding.split('+'); - keymap.forEach((command) => { - const { bindings, id, label, action } = command; - - const keybindings = bindings.map((binding) => { - const keys = binding.split('+'); - - // eslint-disable-next-line no-bitwise - return keys.length > 1 ? getKeyCode(keys[0]) | getKeyCode(keys[1]) : getKeyCode(keys[0]); - }); - - instance.addAction({ - id, - label, - keybindings, - run() { - store.dispatch(action.name, action.params); - return null; - }, - }); + // eslint-disable-next-line no-bitwise + return keys.length > 1 ? getKeyCode(keys[0]) | getKeyCode(keys[1]) : getKeyCode(keys[0]); }); - } - - createModel(file, head = null) { - return this.modelManager.addModel(file, head); - } - - attachModel(model) { - if (isDiffEditorType(this)) { - this.setModel({ - original: model.getOriginalModel(), - modified: model.getModel(), - }); - - return; - } - this.setModel(model.getModel()); + instance.addAction({ + id, + label, + keybindings, + run() { + store.dispatch(action.name, action.params); + return null; + }, + }); + }); +}; - this.updateOptions( - editorOptions.reduce((acc, obj) => { - Object.keys(obj).forEach((key) => { - Object.assign(acc, { - [key]: obj[key](model), - }); - }); - return acc; - }, {}), - ); - } +const renderSideBySide = (domElement) => { + return domElement.offsetWidth >= 700; +}; - attachMergeRequestModel(model) { - this.setModel({ - original: model.getBaseModel(), - modified: model.getModel(), +const updateInstanceDimensions = (instance) => { + instance.layout(); + if (isDiffEditorType(instance)) { + instance.updateOptions({ + renderSideBySide: renderSideBySide(instance.getDomNode()), }); } +}; - updateDimensions() { - this.layout(); - this.updateDiffView(); +export class EditorWebIdeExtension { + /** + * Set up the WebIDE extension for Source Editor + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance + * @param {WebIDEExtensionOptions} setupOptions + */ + onSetup(instance, setupOptions = defaultOptions) { + this.modelManager = setupOptions.modelManager; + this.store = setupOptions.store; + this.file = setupOptions.file; + this.options = setupOptions.options; + + this.disposable = new Disposable(); + this.debouncedUpdate = debounce(() => { + updateInstanceDimensions(instance); + }, UPDATE_DIMENSIONS_DELAY); + + addActions(instance, setupOptions.store); } - setPos({ lineNumber, column }) { - this.revealPositionInCenter({ - lineNumber, - column, - }); - this.setPosition({ - lineNumber, - column, + onUse(instance) { + window.addEventListener('resize', this.debouncedUpdate, false); + + instance.onDidDispose(() => { + this.onUnuse(); }); } - onPositionChange(cb) { - if (!this.onDidChangeCursorPosition) { - return; + onUnuse() { + window.removeEventListener('resize', this.debouncedUpdate); + + // catch any potential errors with disposing the error + // this is mainly for tests caused by elements not existing + try { + this.disposable.dispose(); + } catch (e) { + if (process.env.NODE_ENV !== 'test') { + // eslint-disable-next-line no-console + console.error(e); + } } - - this.disposable.add(this.onDidChangeCursorPosition((e) => cb(this, e))); } - updateDiffView() { - if (!isDiffEditorType(this)) { - return; - } - - this.updateOptions({ - renderSideBySide: EditorWebIdeExtension.renderSideBySide(this.getDomNode()), - }); - } - - replaceSelectedText(text) { - let selection = this.getSelection(); - const range = new Range( - selection.startLineNumber, - selection.startColumn, - selection.endLineNumber, - selection.endColumn, - ); + provides() { + return { + createModel: (instance, file, head = null) => { + return this.modelManager.addModel(file, head); + }, + attachModel: (instance, model) => { + if (isDiffEditorType(instance)) { + instance.setModel({ + original: model.getOriginalModel(), + modified: model.getModel(), + }); - this.executeEdits('', [{ range, text }]); + return; + } - selection = this.getSelection(); - this.setPosition({ lineNumber: selection.endLineNumber, column: selection.endColumn }); - } + instance.setModel(model.getModel()); + + instance.updateOptions( + editorOptions.reduce((acc, obj) => { + Object.keys(obj).forEach((key) => { + Object.assign(acc, { + [key]: obj[key](model), + }); + }); + return acc; + }, {}), + ); + }, + attachMergeRequestModel: (instance, model) => { + instance.setModel({ + original: model.getBaseModel(), + modified: model.getModel(), + }); + }, + updateDimensions: (instance) => updateInstanceDimensions(instance), + setPos: (instance, { lineNumber, column }) => { + instance.revealPositionInCenter({ + lineNumber, + column, + }); + instance.setPosition({ + lineNumber, + column, + }); + }, + onPositionChange: (instance, cb) => { + if (!instance.onDidChangeCursorPosition) { + return; + } - static renderSideBySide(domElement) { - return domElement.offsetWidth >= 700; + this.disposable.add(instance.onDidChangeCursorPosition((e) => cb(instance, e))); + }, + replaceSelectedText: (instance, text) => { + let selection = instance.getSelection(); + const range = new Range( + selection.startLineNumber, + selection.startColumn, + selection.endLineNumber, + selection.endColumn, + ); + + instance.executeEdits('', [{ range, text }]); + + selection = instance.getSelection(); + instance.setPosition({ lineNumber: selection.endLineNumber, column: selection.endColumn }); + }, + }; } } diff --git a/app/assets/javascripts/ide/components/repo_editor.vue b/app/assets/javascripts/ide/components/repo_editor.vue index 2bf99550bf2290..05493db1dffa8e 100644 --- a/app/assets/javascripts/ide/components/repo_editor.vue +++ b/app/assets/javascripts/ide/components/repo_editor.vue @@ -7,6 +7,7 @@ import { EDITOR_CODE_INSTANCE_FN, EDITOR_DIFF_INSTANCE_FN, } from '~/editor/constants'; +import { SourceEditorExtension } from '~/editor/extensions/source_editor_extension_base'; import { EditorWebIdeExtension } from '~/editor/extensions/source_editor_webide_ext'; import SourceEditor from '~/editor/source_editor'; import createFlash from '~/flash'; @@ -302,30 +303,32 @@ export default { ...instanceOptions, ...this.editorOptions, }); - - this.editor.use( - new EditorWebIdeExtension({ - instance: this.editor, - modelManager: this.modelManager, - store: this.$store, - file: this.file, - options: this.editorOptions, - }), - ); + this.editor.use([ + { + definition: SourceEditorExtension, + }, + { + definition: EditorWebIdeExtension, + setupOptions: { + modelManager: this.modelManager, + store: this.$store, + file: this.file, + options: this.editorOptions, + }, + }, + ]); if ( this.fileType === MARKDOWN_FILE_TYPE && this.editor?.getEditorType() === EDITOR_TYPE_CODE && this.previewMarkdownPath ) { - import('~/editor/extensions/source_editor_markdown_ext') - .then(({ EditorMarkdownExtension: MarkdownExtension } = {}) => { - this.editor.use( - new MarkdownExtension({ - instance: this.editor, - previewMarkdownPath: this.previewMarkdownPath, - }), - ); + import('~/editor/extensions/source_editor_markdown_livepreview_ext') + .then(({ EditorMarkdownPreviewExtension: MarkdownLivePreview }) => { + this.editor.use({ + definition: MarkdownLivePreview, + setupOptions: { previewMarkdownPath: this.previewMarkdownPath }, + }); }) .catch((e) => createFlash({ diff --git a/spec/frontend/ide/components/repo_editor_spec.js b/spec/frontend/ide/components/repo_editor_spec.js index c2212eea849868..21e117e042ac83 100644 --- a/spec/frontend/ide/components/repo_editor_spec.js +++ b/spec/frontend/ide/components/repo_editor_spec.js @@ -9,7 +9,7 @@ import waitUsingRealTimer from 'helpers/wait_using_real_timer'; import { exampleConfigs, exampleFiles } from 'jest/ide/lib/editorconfig/mock_data'; import { EDITOR_CODE_INSTANCE_FN, EDITOR_DIFF_INSTANCE_FN } from '~/editor/constants'; import { EditorMarkdownExtension } from '~/editor/extensions/source_editor_markdown_ext'; -import { EditorWebIdeExtension } from '~/editor/extensions/source_editor_webide_ext'; +import { EditorMarkdownPreviewExtension } from '~/editor/extensions/source_editor_markdown_livepreview_ext'; import SourceEditor from '~/editor/source_editor'; import RepoEditor from '~/ide/components/repo_editor.vue'; import { @@ -23,6 +23,8 @@ import service from '~/ide/services'; import { createStoreOptions } from '~/ide/stores'; import axios from '~/lib/utils/axios_utils'; import ContentViewer from '~/vue_shared/components/content_viewer/content_viewer.vue'; +import SourceEditorInstance from '~/editor/source_editor_instance'; +import { spyOnApi } from 'jest/editor/helpers'; import { file } from '../helpers'; const PREVIEW_MARKDOWN_PATH = '/foo/bar/preview_markdown'; @@ -101,6 +103,7 @@ describe('RepoEditor', () => { let createDiffInstanceSpy; let createModelSpy; let applyExtensionSpy; + let extensionsStore; const waitForEditorSetup = () => new Promise((resolve) => { @@ -120,6 +123,7 @@ describe('RepoEditor', () => { }); await waitForPromises(); vm = wrapper.vm; + extensionsStore = wrapper.vm.globalEditor.extensionsStore; jest.spyOn(vm, 'getFileData').mockResolvedValue(); jest.spyOn(vm, 'getRawFileData').mockResolvedValue(); }; @@ -127,28 +131,12 @@ describe('RepoEditor', () => { const findEditor = () => wrapper.find('[data-testid="editor-container"]'); const findTabs = () => wrapper.findAll('.ide-mode-tabs .nav-links li'); const findPreviewTab = () => wrapper.find('[data-testid="preview-tab"]'); - const expectEditorMarkdownExtension = (shouldHaveExtension) => { - if (shouldHaveExtension) { - expect(applyExtensionSpy).toHaveBeenCalledWith( - wrapper.vm.editor, - expect.any(EditorMarkdownExtension), - ); - // TODO: spying on extensions causes Jest to blow up, so we have to assert on - // the public property the extension adds, as opposed to the args passed to the ctor - expect(wrapper.vm.editor.previewMarkdownPath).toBe(PREVIEW_MARKDOWN_PATH); - } else { - expect(applyExtensionSpy).not.toHaveBeenCalledWith( - wrapper.vm.editor, - expect.any(EditorMarkdownExtension), - ); - } - }; beforeEach(() => { createInstanceSpy = jest.spyOn(SourceEditor.prototype, EDITOR_CODE_INSTANCE_FN); createDiffInstanceSpy = jest.spyOn(SourceEditor.prototype, EDITOR_DIFF_INSTANCE_FN); createModelSpy = jest.spyOn(monacoEditor, 'createModel'); - applyExtensionSpy = jest.spyOn(SourceEditor, 'instanceApplyExtension'); + applyExtensionSpy = jest.spyOn(SourceEditorInstance.prototype, 'use'); jest.spyOn(service, 'getFileData').mockResolvedValue(); jest.spyOn(service, 'getRawFileData').mockResolvedValue(); }); @@ -275,14 +263,13 @@ describe('RepoEditor', () => { ); it('installs the WebIDE extension', async () => { - const extensionSpy = jest.spyOn(SourceEditor, 'instanceApplyExtension'); await createComponent(); - expect(extensionSpy).toHaveBeenCalled(); - Reflect.ownKeys(EditorWebIdeExtension.prototype) - .filter((fn) => fn !== 'constructor') - .forEach((fn) => { - expect(vm.editor[fn]).toBe(EditorWebIdeExtension.prototype[fn]); - }); + expect(applyExtensionSpy).toHaveBeenCalled(); + const ideExtensionApi = extensionsStore.get('EditorWebIdeExtension').api; + Reflect.ownKeys(ideExtensionApi).forEach((fn) => { + expect(vm.editor[fn]).toBeDefined(); + expect(vm.editor.methods[fn]).toBe('EditorWebIdeExtension'); + }); }); it.each` @@ -301,7 +288,20 @@ describe('RepoEditor', () => { async ({ activeFile, viewer, shouldHaveMarkdownExtension } = {}) => { await createComponent({ state: { viewer }, activeFile }); - expectEditorMarkdownExtension(shouldHaveMarkdownExtension); + if (shouldHaveMarkdownExtension) { + expect(applyExtensionSpy).toHaveBeenCalledWith({ + definition: EditorMarkdownPreviewExtension, + setupOptions: { previewMarkdownPath: PREVIEW_MARKDOWN_PATH }, + }); + // TODO: spying on extensions causes Jest to blow up, so we have to assert on + // the public property the extension adds, as opposed to the args passed to the ctor + expect(wrapper.vm.editor.markdownPreview.path).toBe(PREVIEW_MARKDOWN_PATH); + } else { + expect(applyExtensionSpy).not.toHaveBeenCalledWith( + wrapper.vm.editor, + expect.any(EditorMarkdownExtension), + ); + } }, ); }); @@ -329,17 +329,26 @@ describe('RepoEditor', () => { expect(vm.model).toBe(existingModel); }); - it('adds callback methods', () => { - jest.spyOn(vm.editor, 'onPositionChange'); - jest.spyOn(vm.model, 'onChange'); - jest.spyOn(vm.model, 'updateOptions'); + it( + 'adds callback methods', + () => { + const ext = extensionsStore.get('EditorWebIdeExtension'); + const posChangeSpy = jest.fn(); + spyOnApi(ext, { + onPositionChange: posChangeSpy, + }); + // jest.spyOn(vm.editor, 'onPositionChange'); + jest.spyOn(vm.model, 'onChange'); + jest.spyOn(vm.model, 'updateOptions'); - vm.setupEditor(); + vm.setupEditor(); - expect(vm.editor.onPositionChange).toHaveBeenCalledTimes(1); - expect(vm.model.onChange).toHaveBeenCalledTimes(1); - expect(vm.model.updateOptions).toHaveBeenCalledWith(vm.rules); - }); + expect(posChangeSpy).toHaveBeenCalledTimes(1); + expect(vm.model.onChange).toHaveBeenCalledTimes(1); + expect(vm.model.updateOptions).toHaveBeenCalledWith(vm.rules); + }, + 'adds callback methods', + ); it('updates state with the value of the model', () => { const newContent = 'As Gregor Samsa\n awoke one morning\n'; @@ -366,53 +375,48 @@ describe('RepoEditor', () => { describe('editor updateDimensions', () => { let updateDimensionsSpy; - let updateDiffViewSpy; beforeEach(async () => { await createComponent(); - updateDimensionsSpy = jest.spyOn(vm.editor, 'updateDimensions'); - updateDiffViewSpy = jest.spyOn(vm.editor, 'updateDiffView').mockImplementation(); + const ext = extensionsStore.get('EditorWebIdeExtension'); + updateDimensionsSpy = jest.fn(); + spyOnApi(ext, { + updateDimensions: updateDimensionsSpy, + }); }); it('calls updateDimensions only when panelResizing is false', async () => { expect(updateDimensionsSpy).not.toHaveBeenCalled(); - expect(updateDiffViewSpy).not.toHaveBeenCalled(); expect(vm.$store.state.panelResizing).toBe(false); // default value vm.$store.state.panelResizing = true; await vm.$nextTick(); expect(updateDimensionsSpy).not.toHaveBeenCalled(); - expect(updateDiffViewSpy).not.toHaveBeenCalled(); vm.$store.state.panelResizing = false; await vm.$nextTick(); expect(updateDimensionsSpy).toHaveBeenCalledTimes(1); - expect(updateDiffViewSpy).toHaveBeenCalledTimes(1); vm.$store.state.panelResizing = true; await vm.$nextTick(); expect(updateDimensionsSpy).toHaveBeenCalledTimes(1); - expect(updateDiffViewSpy).toHaveBeenCalledTimes(1); }); it('calls updateDimensions when rightPane is toggled', async () => { expect(updateDimensionsSpy).not.toHaveBeenCalled(); - expect(updateDiffViewSpy).not.toHaveBeenCalled(); expect(vm.$store.state.rightPane.isOpen).toBe(false); // default value vm.$store.state.rightPane.isOpen = true; await vm.$nextTick(); expect(updateDimensionsSpy).toHaveBeenCalledTimes(1); - expect(updateDiffViewSpy).toHaveBeenCalledTimes(1); vm.$store.state.rightPane.isOpen = false; await vm.$nextTick(); expect(updateDimensionsSpy).toHaveBeenCalledTimes(2); - expect(updateDiffViewSpy).toHaveBeenCalledTimes(2); }); }); @@ -447,7 +451,11 @@ describe('RepoEditor', () => { activeFile: dummyFile.markdown, }); - updateDimensionsSpy = jest.spyOn(vm.editor, 'updateDimensions'); + const ext = extensionsStore.get('EditorWebIdeExtension'); + updateDimensionsSpy = jest.fn(); + spyOnApi(ext, { + updateDimensions: updateDimensionsSpy, + }); changeViewMode(FILE_VIEW_MODE_PREVIEW); await vm.$nextTick(); -- GitLab From 7370c7d9572099750f5ac49f0663db47c6527cf8 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Thu, 9 Dec 2021 16:51:51 +0100 Subject: [PATCH 13/25] Refactor CI Schema Extension --- .../extensions/source_editor_ci_schema_ext.js | 44 ++++++++----------- .../components/editor/text_editor.vue | 2 +- .../source_editor_ci_schema_ext_spec.js | 2 +- .../components/editor/text_editor_spec.js | 5 --- 4 files changed, 20 insertions(+), 33 deletions(-) diff --git a/app/assets/javascripts/editor/extensions/source_editor_ci_schema_ext.js b/app/assets/javascripts/editor/extensions/source_editor_ci_schema_ext.js index 7069568275dc1c..5787bbf802571d 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_ci_schema_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_ci_schema_ext.js @@ -1,32 +1,24 @@ import ciSchemaPath from '~/editor/schema/ci.json'; import { registerSchema } from '~/ide/utils'; -import { SourceEditorExtension } from './source_editor_extension_base'; -export class CiSchemaExtension extends SourceEditorExtension { - /** - * Registers a syntax schema to the editor based on project - * identifier and commit. - * - * The schema is added to the file that is currently edited - * in the editor. - * - * @param {Object} opts - * @param {String} opts.projectNamespace - * @param {String} opts.projectPath - * @param {String?} opts.ref - Current ref. Defaults to main - */ - registerCiSchema() { - // In order for workers loaded from `data://` as the - // ones loaded by monaco editor, we use absolute URLs - // to fetch schema files, hence the `gon.gitlab_url` - // reference. This prevents error: - // "Failed to execute 'fetch' on 'WorkerGlobalScope'" - const absoluteSchemaUrl = gon.gitlab_url + ciSchemaPath; - const modelFileName = this.getModel().uri.path.split('/').pop(); +export class CiSchemaExtension { + // eslint-disable-next-line class-methods-use-this + provides() { + return { + registerCiSchema: (instance) => { + // In order for workers loaded from `data://` as the + // ones loaded by monaco editor, we use absolute URLs + // to fetch schema files, hence the `gon.gitlab_url` + // reference. This prevents error: + // "Failed to execute 'fetch' on 'WorkerGlobalScope'" + const absoluteSchemaUrl = gon.gitlab_url + ciSchemaPath; + const modelFileName = instance.getModel().uri.path.split('/').pop(); - registerSchema({ - uri: absoluteSchemaUrl, - fileMatch: [modelFileName], - }); + registerSchema({ + uri: absoluteSchemaUrl, + fileMatch: [modelFileName], + }); + }, + }; } } diff --git a/app/assets/javascripts/pipeline_editor/components/editor/text_editor.vue b/app/assets/javascripts/pipeline_editor/components/editor/text_editor.vue index 7b8e97b573e77d..92fa411d5afaed 100644 --- a/app/assets/javascripts/pipeline_editor/components/editor/text_editor.vue +++ b/app/assets/javascripts/pipeline_editor/components/editor/text_editor.vue @@ -19,7 +19,7 @@ export default { if (this.glFeatures.schemaLinting) { const editorInstance = this.$refs.editor.getEditor(); - editorInstance.use(new CiSchemaExtension({ instance: editorInstance })); + editorInstance.use({ definition: CiSchemaExtension }); editorInstance.registerCiSchema(); } }, diff --git a/spec/frontend/editor/source_editor_ci_schema_ext_spec.js b/spec/frontend/editor/source_editor_ci_schema_ext_spec.js index 8a0d1ecf1af7b3..5eaac9e9ef947b 100644 --- a/spec/frontend/editor/source_editor_ci_schema_ext_spec.js +++ b/spec/frontend/editor/source_editor_ci_schema_ext_spec.js @@ -23,7 +23,7 @@ describe('~/editor/editor_ci_config_ext', () => { blobPath, blobContent: '', }); - instance.use(new CiSchemaExtension()); + instance.use({ definition: CiSchemaExtension }); }; beforeAll(() => { diff --git a/spec/frontend/pipeline_editor/components/editor/text_editor_spec.js b/spec/frontend/pipeline_editor/components/editor/text_editor_spec.js index a43da4b0f19428..cab4810cbf1cd1 100644 --- a/spec/frontend/pipeline_editor/components/editor/text_editor_spec.js +++ b/spec/frontend/pipeline_editor/components/editor/text_editor_spec.js @@ -1,7 +1,6 @@ import { shallowMount } from '@vue/test-utils'; import { EDITOR_READY_EVENT } from '~/editor/constants'; -import { SourceEditorExtension } from '~/editor/extensions/source_editor_extension_base'; import TextEditor from '~/pipeline_editor/components/editor/text_editor.vue'; import { mockCiConfigPath, @@ -59,10 +58,6 @@ describe('Pipeline Editor | Text editor component', () => { const findEditor = () => wrapper.findComponent(MockSourceEditor); - beforeEach(() => { - SourceEditorExtension.deferRerender = jest.fn(); - }); - afterEach(() => { wrapper.destroy(); -- GitLab From 8462653737f613214cd7c0aaf09d0fab0ccebf20 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Fri, 10 Dec 2021 00:10:54 +0100 Subject: [PATCH 14/25] The new spyOnApi helper method to work with SE extensions --- spec/frontend/editor/helpers.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/spec/frontend/editor/helpers.js b/spec/frontend/editor/helpers.js index dfdbf6913e576c..902a1944187ac4 100644 --- a/spec/frontend/editor/helpers.js +++ b/spec/frontend/editor/helpers.js @@ -4,11 +4,9 @@ export const spyOnApi = (extension, spiesObj = {}) => { const origApi = extension.api; if (extension?.obj) { - jest.spyOn(extension.obj, 'provides').mockImplementation(() => { - return { - ...origApi, - ...spiesObj, - }; + jest.spyOn(extension.obj, 'provides').mockReturnValue({ + ...origApi, + ...spiesObj, }); } }; -- GitLab From 112a7fea2a1787d5eb16d7e21fb1036f50f8faab Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Fri, 10 Dec 2021 00:11:35 +0100 Subject: [PATCH 15/25] Refactor YAML Extension --- .../extensions/source_editor_yaml_ext.js | 274 +++++++++--------- .../editor/source_editor_yaml_ext_spec.js | 73 +++-- 2 files changed, 188 insertions(+), 159 deletions(-) diff --git a/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js b/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js index 212e09c8724d72..c4210ee46b01a6 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js @@ -1,50 +1,42 @@ +/** + * A Yaml Editor Extension options for Source Editor + * @typedef {Object} YamlEditorExtensionOptions + * @property { boolean } enableComments Convert model nodes with the comment + * pattern to comments? + * @property { string } highlightPath Add a line highlight to the + * node specified by this e.g. `"foo.bar[0]"` + * @property { * } model Any JS Object that will be stringified and used as the + * editor's value. Equivalent to using `setDataModel()` + * @property options SourceEditorExtension Options + */ + import { toPath } from 'lodash'; import { parseDocument, Document, visit, isScalar, isCollection, isMap } from 'yaml'; import { findPair } from 'yaml/util'; -import { SourceEditorExtension } from '~/editor/extensions/source_editor_extension_base'; -export class YamlEditorExtension extends SourceEditorExtension { +export class YamlEditorExtension { /** * Extends the source editor with capabilities for yaml files. * - * @param { Instance } instance Source Editor Instance - * @param { boolean } enableComments Convert model nodes with the comment - * pattern to comments? - * @param { string } highlightPath Add a line highlight to the - * node specified by this e.g. `"foo.bar[0]"` - * @param { * } model Any JS Object that will be stringified and used as the - * editor's value. Equivalent to using `setDataModel()` - * @param options SourceEditorExtension Options + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance + * @param {YamlEditorExtensionOptions} setupOptions */ - constructor({ - instance, - enableComments = false, - highlightPath = null, - model = null, - ...options - } = {}) { - super({ - instance, - options: { - ...options, - enableComments, - highlightPath, - }, - }); + onSetup(instance, setupOptions = {}) { + const { enableComments = false, highlightPath = null, model = null } = setupOptions; + this.enableComments = enableComments; + this.highlightPath = highlightPath; + this.model = model; if (model) { - YamlEditorExtension.initFromModel(instance, model); + this.initFromModel(instance, model); } instance.onDidChangeModelContent(() => instance.onUpdate()); } - /** - * @private - */ - static initFromModel(instance, model) { + initFromModel(instance, model) { const doc = new Document(model); - if (instance.options.enableComments) { + if (this.enableComments) { YamlEditorExtension.transformComments(doc); } instance.setValue(doc.toString()); @@ -160,110 +152,13 @@ export class YamlEditorExtension extends SourceEditorExtension { return doc; } - /** - * Get the editor's value parsed as a `Document` as defined by the `yaml` - * package - * @returns {Document} - */ - getDoc() { - return parseDocument(this.getValue()); - } - - /** - * Accepts a `Document` as defined by the `yaml` package and - * sets the Editor's value to a stringified version of it. - * @param { Document } doc - */ - setDoc(doc) { - if (this.options.enableComments) { - YamlEditorExtension.transformComments(doc); - } - - if (!this.getValue()) { - this.setValue(doc.toString()); - } else { - this.updateValue(doc.toString()); - } - } - - /** - * Returns the parsed value of the Editor's content as JS. - * @returns {*} - */ - getDataModel() { - return this.getDoc().toJS(); - } - - /** - * Accepts any JS Object and sets the Editor's value to a stringified version - * of that value. - * - * @param value - */ - setDataModel(value) { - this.setDoc(new Document(value)); - } - - /** - * Method to be executed when the Editor's was updated - */ - onUpdate() { - if (this.options.highlightPath) { - this.highlight(this.options.highlightPath); - } - } - - /** - * Set the editors content to the input without recreating the content model. - * - * @param blob - */ - updateValue(blob) { - // Using applyEdits() instead of setValue() ensures that tokens such as - // highlighted lines aren't deleted/recreated which causes a flicker. - const model = this.getModel(); - model.applyEdits([ - { - // A nice improvement would be to replace getFullModelRange() with - // a range of the actual diff, avoiding re-formatting the document, - // but that's something for a later iteration. - range: model.getFullModelRange(), - text: blob, - }, - ]); - } - - /** - * Add a line highlight style to the node specified by the path. - * - * @param {string|null|false} path A path to a node of the Editor's value, - * e.g. `"foo.bar[0]"`. If the value is falsy, this will remove all - * highlights. - */ - highlight(path) { - if (this.options.highlightPath === path) return; - if (!path) { - SourceEditorExtension.removeHighlights(this); - } else { - const res = this.locate(path); - SourceEditorExtension.highlightLines(this, res); - } - this.options.highlightPath = path || null; + static getDoc(instance) { + return parseDocument(instance.getValue()); } - /** - * Return the line numbers of a certain node identified by `path` within - * the yaml. - * - * @param {string} path A path to a node, eg. `foo.bar[0]` - * @returns {number[]} Array following the schema `[firstLine, lastLine]` - * (both inclusive) - * - * @throws {Error} Will throw if the path is not found inside the document - */ - locate(path) { + static locate(instance, path) { if (!path) throw Error(`No path provided.`); - const blob = this.getValue(); + const blob = instance.getValue(); const doc = parseDocument(blob); const pathArray = toPath(path); @@ -290,4 +185,119 @@ export class YamlEditorExtension extends SourceEditorExtension { const endLine = (endSlice.match(/\n/g) || []).length; return [startLine, endLine]; } + + setDoc(instance, doc) { + if (this.enableComments) { + YamlEditorExtension.transformComments(doc); + } + + if (!instance.getValue()) { + instance.setValue(doc.toString()); + } else { + instance.updateValue(doc.toString()); + } + } + + highlight(instance, path) { + // IMPORTANT + // removeHighlight and highlightLines both come from + // SourceEditorExtension. So it has to be installed prior to this extension + if (this.highlightPath === path) return; + if (!path) { + instance.removeHighlights(); + } else { + const res = YamlEditorExtension.locate(instance, path); + instance.highlightLines(res); + } + this.highlightPath = path || null; + } + + provides() { + return { + /** + * Get the editor's value parsed as a `Document` as defined by the `yaml` + * package + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance + * @returns {Document} + */ + getDoc: (instance) => YamlEditorExtension.getDoc(instance), + + /** + * Accepts a `Document` as defined by the `yaml` package and + * sets the Editor's value to a stringified version of it. + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance + * @param { Document } doc + */ + setDoc: (instance, doc) => this.setDoc(instance, doc), + + /** + * Returns the parsed value of the Editor's content as JS. + * @returns {*} + */ + getDataModel: (instance) => YamlEditorExtension.getDoc(instance).toJS(), + + /** + * Accepts any JS Object and sets the Editor's value to a stringified version + * of that value. + * + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance + * @param value + */ + setDataModel: (instance, value) => this.setDoc(instance, new Document(value)), + + /** + * Method to be executed when the Editor's was updated + */ + onUpdate: (instance) => { + if (this.highlightPath) { + this.highlight(instance, this.highlightPath); + } + }, + + /** + * Set the editors content to the input without recreating the content model. + * + * @param blob + */ + updateValue: (instance, blob) => { + // Using applyEdits() instead of setValue() ensures that tokens such as + // highlighted lines aren't deleted/recreated which causes a flicker. + const model = instance.getModel(); + model.applyEdits([ + { + // A nice improvement would be to replace getFullModelRange() with + // a range of the actual diff, avoiding re-formatting the document, + // but that's something for a later iteration. + range: model.getFullModelRange(), + text: blob, + }, + ]); + }, + + /** + * Add a line highlight style to the node specified by the path. + * + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance + * @param {string|null|false} path A path to a node of the Editor's value, + * e.g. `"foo.bar[0]"`. If the value is falsy, this will remove all + * highlights. + */ + highlight: (instance, path) => this.highlight(instance, path), + + /** + * Return the line numbers of a certain node identified by `path` within + * the yaml. + * + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance + * @param {string} path A path to a node, eg. `foo.bar[0]` + * @returns {number[]} Array following the schema `[firstLine, lastLine]` + * (both inclusive) + * + * @throws {Error} Will throw if the path is not found inside the document + */ + locate: (instance, path) => YamlEditorExtension.locate(instance, path), + + initFromModel: (instance, model) => this.initFromModel(instance, model), + }; + } } diff --git a/spec/frontend/editor/source_editor_yaml_ext_spec.js b/spec/frontend/editor/source_editor_yaml_ext_spec.js index 97d2b0b21d0062..d2690d271bd38e 100644 --- a/spec/frontend/editor/source_editor_yaml_ext_spec.js +++ b/spec/frontend/editor/source_editor_yaml_ext_spec.js @@ -2,6 +2,10 @@ import { Document } from 'yaml'; import SourceEditor from '~/editor/source_editor'; import { YamlEditorExtension } from '~/editor/extensions/source_editor_yaml_ext'; import { SourceEditorExtension } from '~/editor/extensions/source_editor_extension_base'; +import { spyOnApi } from 'jest/editor/helpers'; + +let baseExtension; +let yamlExtension; const getEditorInstance = (editorInstanceOptions = {}) => { setFixtures('
'); @@ -16,7 +20,10 @@ const getEditorInstance = (editorInstanceOptions = {}) => { const getEditorInstanceWithExtension = (extensionOptions = {}, editorInstanceOptions = {}) => { setFixtures('
'); const instance = getEditorInstance(editorInstanceOptions); - instance.use(new YamlEditorExtension({ instance, ...extensionOptions })); + [baseExtension, yamlExtension] = instance.use([ + { definition: SourceEditorExtension }, + { definition: YamlEditorExtension, setupOptions: extensionOptions }, + ]); // Remove the below once // https://gitlab.com/gitlab-org/gitlab/-/issues/325992 is resolved @@ -29,19 +36,15 @@ const getEditorInstanceWithExtension = (extensionOptions = {}, editorInstanceOpt describe('YamlCreatorExtension', () => { describe('constructor', () => { - it('saves constructor options', () => { + it('saves setupOptions options on the extension, but does not expose those to instance', () => { const instance = getEditorInstanceWithExtension({ highlightPath: 'foo', enableComments: true, }); - expect(instance).toEqual( - expect.objectContaining({ - options: expect.objectContaining({ - highlightPath: 'foo', - enableComments: true, - }), - }), - ); + expect(yamlExtension.obj.highlightPath).toBe('foo'); + expect(yamlExtension.obj.enableComments).toBe(true); + expect(instance.highlightPath).toBeUndefined(); + expect(instance.enableComments).toBeUndefined(); }); it('dumps values loaded with the model constructor options', () => { @@ -55,7 +58,7 @@ describe('YamlCreatorExtension', () => { it('registers the onUpdate() function', () => { const instance = getEditorInstance(); const onDidChangeModelContent = jest.spyOn(instance, 'onDidChangeModelContent'); - instance.use(new YamlEditorExtension({ instance })); + instance.use({ definition: YamlEditorExtension }); expect(onDidChangeModelContent).toHaveBeenCalledWith(expect.any(Function)); }); @@ -82,21 +85,21 @@ describe('YamlCreatorExtension', () => { it('should call transformComments if enableComments is true', () => { const instance = getEditorInstanceWithExtension({ enableComments: true }); const transformComments = jest.spyOn(YamlEditorExtension, 'transformComments'); - YamlEditorExtension.initFromModel(instance, model); + instance.initFromModel(model); expect(transformComments).toHaveBeenCalled(); }); it('should not call transformComments if enableComments is false', () => { const instance = getEditorInstanceWithExtension({ enableComments: false }); const transformComments = jest.spyOn(YamlEditorExtension, 'transformComments'); - YamlEditorExtension.initFromModel(instance, model); + instance.initFromModel(model); expect(transformComments).not.toHaveBeenCalled(); }); it('should call setValue with the stringified model', () => { const instance = getEditorInstanceWithExtension(); const setValue = jest.spyOn(instance, 'setValue'); - YamlEditorExtension.initFromModel(instance, model); + instance.initFromModel(model); expect(setValue).toHaveBeenCalledWith(doc.toString()); }); }); @@ -240,26 +243,35 @@ foo: it("should call setValue with the stringified doc if the editor's value is empty", () => { const instance = getEditorInstanceWithExtension(); const setValue = jest.spyOn(instance, 'setValue'); - const updateValue = jest.spyOn(instance, 'updateValue'); + const updateValueSpy = jest.fn(); + spyOnApi(yamlExtension, { + updateValue: updateValueSpy, + }); instance.setDoc(doc); expect(setValue).toHaveBeenCalledWith(doc.toString()); - expect(updateValue).not.toHaveBeenCalled(); + expect(updateValueSpy).not.toHaveBeenCalled(); }); it("should call updateValue with the stringified doc if the editor's value is not empty", () => { const instance = getEditorInstanceWithExtension({}, { value: 'asjkdhkasjdh' }); const setValue = jest.spyOn(instance, 'setValue'); - const updateValue = jest.spyOn(instance, 'updateValue'); + const updateValueSpy = jest.fn(); + spyOnApi(yamlExtension, { + updateValue: updateValueSpy, + }); instance.setDoc(doc); expect(setValue).not.toHaveBeenCalled(); - expect(updateValue).toHaveBeenCalledWith(doc.toString()); + expect(updateValueSpy).toHaveBeenCalledWith(instance, doc.toString()); }); it('should trigger the onUpdate method', () => { const instance = getEditorInstanceWithExtension(); - const onUpdate = jest.spyOn(instance, 'onUpdate'); + const onUpdateSpy = jest.fn(); + spyOnApi(yamlExtension, { + onUpdate: onUpdateSpy, + }); instance.setDoc(doc); - expect(onUpdate).toHaveBeenCalled(); + expect(onUpdateSpy).toHaveBeenCalled(); }); }); @@ -320,9 +332,12 @@ foo: it('calls highlight', () => { const highlightPath = 'foo'; const instance = getEditorInstanceWithExtension({ highlightPath }); - instance.highlight = jest.fn(); + // Here we do not spy on the public API method of the extension, but rather + // the public method of the extension's instance. + // This is required based on how `onUpdate` works + const highlightSpy = jest.spyOn(yamlExtension.obj, 'highlight'); instance.onUpdate(); - expect(instance.highlight).toHaveBeenCalledWith(highlightPath); + expect(highlightSpy).toHaveBeenCalledWith(instance, highlightPath); }); }); @@ -350,8 +365,12 @@ foo: beforeEach(() => { instance = getEditorInstanceWithExtension({ highlightPath: highlightPathOnSetup }, { value }); - highlightLinesSpy = jest.spyOn(SourceEditorExtension, 'highlightLines'); - removeHighlightsSpy = jest.spyOn(SourceEditorExtension, 'removeHighlights'); + highlightLinesSpy = jest.fn(); + removeHighlightsSpy = jest.fn(); + spyOnApi(baseExtension, { + highlightLines: highlightLinesSpy, + removeHighlights: removeHighlightsSpy, + }); }); afterEach(() => { @@ -361,7 +380,7 @@ foo: it('saves the highlighted path in highlightPath', () => { const path = 'foo.bar'; instance.highlight(path); - expect(instance.options.highlightPath).toEqual(path); + expect(yamlExtension.obj.highlightPath).toEqual(path); }); it('calls highlightLines with a number of lines', () => { @@ -374,14 +393,14 @@ foo: instance.highlight(null); expect(removeHighlightsSpy).toHaveBeenCalledWith(instance); expect(highlightLinesSpy).not.toHaveBeenCalled(); - expect(instance.options.highlightPath).toBeNull(); + expect(yamlExtension.obj.highlightPath).toBeNull(); }); it('throws an error if path is invalid and does not change the highlighted path', () => { expect(() => instance.highlight('invalidPath[0]')).toThrow( 'The node invalidPath[0] could not be found inside the document.', ); - expect(instance.options.highlightPath).toEqual(highlightPathOnSetup); + expect(yamlExtension.obj.highlightPath).toEqual(highlightPathOnSetup); expect(highlightLinesSpy).not.toHaveBeenCalled(); expect(removeHighlightsSpy).not.toHaveBeenCalled(); }); -- GitLab From 6d18f4a0858a1bac7243d72f3d8140666597c730 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Fri, 10 Dec 2021 01:16:20 +0100 Subject: [PATCH 16/25] Switched to new Source Editor Instance --- spec/frontend/editor/source_editor_spec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/frontend/editor/source_editor_spec.js b/spec/frontend/editor/source_editor_spec.js index 11c0a1f969bb8e..bc53202c919a69 100644 --- a/spec/frontend/editor/source_editor_spec.js +++ b/spec/frontend/editor/source_editor_spec.js @@ -355,6 +355,7 @@ describe('Base editor', () => { return { setModel: jest.fn(), onDidDispose: jest.fn(), + layout: jest.fn(), }; }); const eventSpy = jest.fn(); -- GitLab From e41e1680c8f4460cb46de4e98c2b6dbe22628477 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Mon, 13 Dec 2021 14:18:12 +0100 Subject: [PATCH 17/25] Fix the type on fn's name As per review --- .../extensions/source_editor_markdown_livepreview_ext.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js b/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js index 921a6028b95663..9a1fcd333f39ec 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js @@ -13,7 +13,7 @@ import { EXTENSION_MARKDOWN_PREVIEW_UPDATE_DELAY, } from '../constants'; -const fetchtPreview = (text, previewMarkdownPath) => { +const fetchPreview = (text, previewMarkdownPath) => { return axios .post(previewMarkdownPath, { text, @@ -88,7 +88,7 @@ export class EditorMarkdownPreviewExtension { fetchPreview(instance) { const { el: previewEl } = this.preview; - fetchtPreview(instance.getValue(), this.preview.path) + fetchPreview(instance.getValue(), this.preview.path) .then((data) => { previewEl.innerHTML = sanitize(data); syntaxHighlight(previewEl.querySelectorAll('.js-syntax-highlight')); -- GitLab From d246b156a85d6087ffadc8f99ee9e340267a187d Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Mon, 13 Dec 2021 14:54:51 +0100 Subject: [PATCH 18/25] Removed redundant test As per review --- .../ide/components/repo_editor_spec.js | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/spec/frontend/ide/components/repo_editor_spec.js b/spec/frontend/ide/components/repo_editor_spec.js index 21e117e042ac83..93bc3a0e11c49a 100644 --- a/spec/frontend/ide/components/repo_editor_spec.js +++ b/spec/frontend/ide/components/repo_editor_spec.js @@ -329,27 +329,6 @@ describe('RepoEditor', () => { expect(vm.model).toBe(existingModel); }); - it( - 'adds callback methods', - () => { - const ext = extensionsStore.get('EditorWebIdeExtension'); - const posChangeSpy = jest.fn(); - spyOnApi(ext, { - onPositionChange: posChangeSpy, - }); - // jest.spyOn(vm.editor, 'onPositionChange'); - jest.spyOn(vm.model, 'onChange'); - jest.spyOn(vm.model, 'updateOptions'); - - vm.setupEditor(); - - expect(posChangeSpy).toHaveBeenCalledTimes(1); - expect(vm.model.onChange).toHaveBeenCalledTimes(1); - expect(vm.model.updateOptions).toHaveBeenCalledWith(vm.rules); - }, - 'adds callback methods', - ); - it('updates state with the value of the model', () => { const newContent = 'As Gregor Samsa\n awoke one morning\n'; vm.model.setValue(newContent); -- GitLab From e446e7ff685aa7f5b1e631f2abb71923c3b0a71f Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Mon, 13 Dec 2021 14:57:17 +0100 Subject: [PATCH 19/25] Introduced async/await for promise. As per review --- app/assets/javascripts/blob_edit/edit_blob.js | 47 +++++++++---------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/blob_edit/edit_blob.js b/app/assets/javascripts/blob_edit/edit_blob.js index 43a0bbef948406..ee2f6cfb46c8ad 100644 --- a/app/assets/javascripts/blob_edit/edit_blob.js +++ b/app/assets/javascripts/blob_edit/edit_blob.js @@ -27,32 +27,29 @@ export default class EditBlob { this.editor.focus(); } - fetchMarkdownExtension() { - return Promise.all([ - import('~/editor/extensions/source_editor_markdown_ext'), - import('~/editor/extensions/source_editor_markdown_livepreview_ext'), - ]) - .then( - ([ - { EditorMarkdownExtension: MarkdownExtension }, - { EditorMarkdownPreviewExtension: MarkdownLivePreview }, - ]) => { - this.editor.use([ - { definition: MarkdownExtension }, - { - definition: MarkdownLivePreview, - setupOptions: { previewMarkdownPath: this.options.previewMarkdownPath }, - }, - ]); - this.hasMarkdownExtension = true; - addEditorMarkdownListeners(this.editor); + async fetchMarkdownExtension() { + try { + const [ + { EditorMarkdownExtension: MarkdownExtension }, + { EditorMarkdownPreviewExtension: MarkdownLivePreview }, + ] = await Promise.all([ + import('~/editor/extensions/source_editor_markdown_ext'), + import('~/editor/extensions/source_editor_markdown_livepreview_ext'), + ]); + this.editor.use([ + { definition: MarkdownExtension }, + { + definition: MarkdownLivePreview, + setupOptions: { previewMarkdownPath: this.options.previewMarkdownPath }, }, - ) - .catch((e) => - createFlash({ - message: `${BLOB_EDITOR_ERROR}: ${e}`, - }), - ); + ]); + } catch (e) { + createFlash({ + message: `${BLOB_EDITOR_ERROR}: ${e}`, + }); + } + this.hasMarkdownExtension = true; + addEditorMarkdownListeners(this.editor); } configureMonacoEditor() { -- GitLab From 256af27cf97044c18c047e31a465141862955682 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Mon, 13 Dec 2021 15:25:16 +0100 Subject: [PATCH 20/25] Check for the fn type rather than existence As per review --- .../javascripts/editor/extensions/source_editor_webide_ext.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js b/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js index ba622bef20ebc7..2313b37d060f3f 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js @@ -157,7 +157,7 @@ export class EditorWebIdeExtension { }); }, onPositionChange: (instance, cb) => { - if (!instance.onDidChangeCursorPosition) { + if (typeof instance.onDidChangeCursorPosition !== 'function') { return; } -- GitLab From 2976e1f40ec90836dabba50f0de27f2282b0c5bd Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Mon, 13 Dec 2021 16:52:01 +0100 Subject: [PATCH 21/25] Added missing JSDoc param definition As per review --- .../javascripts/editor/extensions/source_editor_yaml_ext.js | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js b/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js index c4210ee46b01a6..2cf71aa6fddba9 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js @@ -257,6 +257,7 @@ export class YamlEditorExtension { /** * Set the editors content to the input without recreating the content model. * + * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance * @param blob */ updateValue: (instance, blob) => { -- GitLab From 561cfb11b8ed54c98ac59bb2d6b5fc81b3460a23 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Mon, 13 Dec 2021 17:01:44 +0100 Subject: [PATCH 22/25] Abstract CSS classes into the constants As per review --- app/assets/javascripts/editor/constants.js | 4 ++++ .../source_editor_extension_base.js | 22 ++++++++++++------- .../source_editor_extension_base_spec.js | 17 +++++++++----- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/editor/constants.js b/app/assets/javascripts/editor/constants.js index e855e304d278c3..2ae9c377683b8a 100644 --- a/app/assets/javascripts/editor/constants.js +++ b/app/assets/javascripts/editor/constants.js @@ -42,6 +42,10 @@ export const EDITOR_EXTENSION_STORE_IS_MISSING_ERROR = s__( // EXTENSIONS' CONSTANTS // +// Source Editor Base Extension +export const EXTENSION_BASE_LINE_LINK_ANCHOR_CLASS = 'link-anchor'; +export const EXTENSION_BASE_LINE_NUMBERS_CLASS = 'line-numbers'; + // For CI config schemas the filename must match // '*.gitlab-ci.yml' regardless of project configuration. // https://gitlab.com/gitlab-org/gitlab/-/issues/293641 diff --git a/app/assets/javascripts/editor/extensions/source_editor_extension_base.js b/app/assets/javascripts/editor/extensions/source_editor_extension_base.js index 34104b0febb683..d714bc55afb8fe 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_extension_base.js +++ b/app/assets/javascripts/editor/extensions/source_editor_extension_base.js @@ -1,12 +1,16 @@ import { Range } from 'monaco-editor'; -import { EDITOR_TYPE_CODE } from '../constants'; +import { + EDITOR_TYPE_CODE, + EXTENSION_BASE_LINE_LINK_ANCHOR_CLASS, + EXTENSION_BASE_LINE_NUMBERS_CLASS, +} from '../constants'; const hashRegexp = new RegExp('#?L', 'g'); const createAnchor = (href) => { const fragment = new DocumentFragment(); const el = document.createElement('a'); - el.classList.add('link-anchor'); + el.classList.add(EXTENSION_BASE_LINE_LINK_ANCHOR_CLASS); el.href = href; fragment.appendChild(el); el.addEventListener('contextmenu', (e) => { @@ -26,13 +30,13 @@ export class SourceEditorExtension { static onMouseMoveHandler(e) { const target = e.target.element; - if (target.classList.contains('line-numbers')) { + if (target.classList.contains(EXTENSION_BASE_LINE_NUMBERS_CLASS)) { const lineNum = e.target.position.lineNumber; const hrefAttr = `#L${lineNum}`; - let el = target.querySelector('a'); - if (!el) { - el = createAnchor(hrefAttr); - target.appendChild(el); + let lineLink = target.querySelector('a'); + if (!lineLink) { + lineLink = createAnchor(hrefAttr); + target.appendChild(lineLink); } } } @@ -40,7 +44,9 @@ export class SourceEditorExtension { static setupLineLinking(instance) { instance.onMouseMove(SourceEditorExtension.onMouseMoveHandler); instance.onMouseDown((e) => { - const isCorrectAnchor = e.target.element.classList.contains('link-anchor'); + const isCorrectAnchor = e.target.element.classList.contains( + EXTENSION_BASE_LINE_LINK_ANCHOR_CLASS, + ); if (!isCorrectAnchor) { return; } diff --git a/spec/frontend/editor/source_editor_extension_base_spec.js b/spec/frontend/editor/source_editor_extension_base_spec.js index 6ec593ba8d2d45..6606557fd1f338 100644 --- a/spec/frontend/editor/source_editor_extension_base_spec.js +++ b/spec/frontend/editor/source_editor_extension_base_spec.js @@ -1,7 +1,12 @@ import { Range } from 'monaco-editor'; import { useFakeRequestAnimationFrame } from 'helpers/fake_request_animation_frame'; import setWindowLocation from 'helpers/set_window_location_helper'; -import { EDITOR_TYPE_CODE, EDITOR_TYPE_DIFF } from '~/editor/constants'; +import { + EDITOR_TYPE_CODE, + EDITOR_TYPE_DIFF, + EXTENSION_BASE_LINE_LINK_ANCHOR_CLASS, + EXTENSION_BASE_LINE_NUMBERS_CLASS, +} from '~/editor/constants'; import { SourceEditorExtension } from '~/editor/extensions/source_editor_extension_base'; import EditorInstance from '~/editor/source_editor_instance'; @@ -10,12 +15,12 @@ describe('The basis for an Source Editor extension', () => { let event; const findLine = (num) => { - return document.querySelector(`.line-numbers:nth-child(${num})`); + return document.querySelector(`.${EXTENSION_BASE_LINE_NUMBERS_CLASS}:nth-child(${num})`); }; const generateLines = () => { let res = ''; for (let line = 1, lines = 5; line <= lines; line += 1) { - res += `
${line}
`; + res += `
${line}
`; } return res; }; @@ -201,9 +206,9 @@ describe('The basis for an Source Editor extension', () => { }); it.each` - desc | eventTrigger | shouldRemove - ${'does not remove the line decorations if the event is triggered on a wrong node'} | ${null} | ${false} - ${'removes existing line decorations when clicking a line number'} | ${'.link-anchor'} | ${true} + desc | eventTrigger | shouldRemove + ${'does not remove the line decorations if the event is triggered on a wrong node'} | ${null} | ${false} + ${'removes existing line decorations when clicking a line number'} | ${`.${EXTENSION_BASE_LINE_LINK_ANCHOR_CLASS}`} | ${true} `('$desc', ({ eventTrigger, shouldRemove } = {}) => { event = generateEventMock({ el: eventTrigger ? document.querySelector(eventTrigger) : null }); instance.onMouseDown.mockImplementation((fn) => { -- GitLab From 809845a29b2021f5eb63ed70f44052e060434bdf Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Mon, 13 Dec 2021 17:05:50 +0100 Subject: [PATCH 23/25] Removed default `undefined` value for an argument As per review --- .../javascripts/editor/extensions/source_editor_markdown_ext.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js b/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js index 35f814c3949378..106669432b102d 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js @@ -22,7 +22,7 @@ export class EditorMarkdownExtension { } return text; }, - replaceSelectedText: (instance, text, select = undefined) => { + replaceSelectedText: (instance, text, select) => { const forceMoveMarkers = !select; instance.executeEdits('', [{ range: instance.getSelection(), text, forceMoveMarkers }]); }, -- GitLab From 704634605c18d49b4238181df1967e2a8527a000 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Mon, 13 Dec 2021 17:11:34 +0100 Subject: [PATCH 24/25] Abstracted variable for re-use As per review --- spec/frontend/editor/source_editor_yaml_ext_spec.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/frontend/editor/source_editor_yaml_ext_spec.js b/spec/frontend/editor/source_editor_yaml_ext_spec.js index d2690d271bd38e..a861d9c7a455af 100644 --- a/spec/frontend/editor/source_editor_yaml_ext_spec.js +++ b/spec/frontend/editor/source_editor_yaml_ext_spec.js @@ -37,11 +37,12 @@ const getEditorInstanceWithExtension = (extensionOptions = {}, editorInstanceOpt describe('YamlCreatorExtension', () => { describe('constructor', () => { it('saves setupOptions options on the extension, but does not expose those to instance', () => { + const highlightPath = 'foo'; const instance = getEditorInstanceWithExtension({ - highlightPath: 'foo', + highlightPath, enableComments: true, }); - expect(yamlExtension.obj.highlightPath).toBe('foo'); + expect(yamlExtension.obj.highlightPath).toBe(highlightPath); expect(yamlExtension.obj.enableComments).toBe(true); expect(instance.highlightPath).toBeUndefined(); expect(instance.enableComments).toBeUndefined(); -- GitLab From 01feff215ff7a0b0e1348547b8facd661a75abd7 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Tue, 14 Dec 2021 15:36:29 +0100 Subject: [PATCH 25/25] Explicitly define extension's name To avoid minifiers to mingle the name of the extenson, we have to be explcicit about it to not get into regressions and other side-effects --- .../example_source_editor_extension.js | 10 +++++++ .../extensions/source_editor_ci_schema_ext.js | 3 ++ .../source_editor_extension_base.js | 4 +++ .../source_editor_file_template_ext.js | 4 +++ .../extensions/source_editor_markdown_ext.js | 4 +++ .../source_editor_markdown_livepreview_ext.js | 4 +++ .../extensions/source_editor_webide_ext.js | 4 +++ .../extensions/source_editor_yaml_ext.js | 4 +++ .../editor/source_editor_extension.js | 2 +- .../editor/source_editor_instance.js | 30 +++++++++---------- spec/frontend/editor/helpers.js | 11 +++++++ .../editor/source_editor_extension_spec.js | 2 +- .../editor/source_editor_instance_spec.js | 4 +++ .../ide/components/repo_editor_spec.js | 8 ++--- 14 files changed, 73 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/editor/extensions/example_source_editor_extension.js b/app/assets/javascripts/editor/extensions/example_source_editor_extension.js index 33be6cf9e5d940..52e2bb0b5ffcac 100644 --- a/app/assets/javascripts/editor/extensions/example_source_editor_extension.js +++ b/app/assets/javascripts/editor/extensions/example_source_editor_extension.js @@ -6,6 +6,16 @@ // export class MyFancyExtension { + /** + * A required getter returning the extension's name + * We have to provide it for every extension instead of relying on the built-in + * `name` prop because the prop does not survive the webpack's minification + * and the name mangling. + * @returns {string} + */ + static get extensionName() { + return 'MyFancyExtension'; + } /** * THE LIFE-CYCLE CALLBACKS */ diff --git a/app/assets/javascripts/editor/extensions/source_editor_ci_schema_ext.js b/app/assets/javascripts/editor/extensions/source_editor_ci_schema_ext.js index 5787bbf802571d..0290bb84b5f6dd 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_ci_schema_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_ci_schema_ext.js @@ -2,6 +2,9 @@ import ciSchemaPath from '~/editor/schema/ci.json'; import { registerSchema } from '~/ide/utils'; export class CiSchemaExtension { + static get extensionName() { + return 'CiSchema'; + } // eslint-disable-next-line class-methods-use-this provides() { return { diff --git a/app/assets/javascripts/editor/extensions/source_editor_extension_base.js b/app/assets/javascripts/editor/extensions/source_editor_extension_base.js index d714bc55afb8fe..3aa19df964cdf4 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_extension_base.js +++ b/app/assets/javascripts/editor/extensions/source_editor_extension_base.js @@ -20,6 +20,10 @@ const createAnchor = (href) => { }; export class SourceEditorExtension { + static get extensionName() { + return 'BaseExtension'; + } + // eslint-disable-next-line class-methods-use-this onUse(instance) { SourceEditorExtension.highlightLines(instance); diff --git a/app/assets/javascripts/editor/extensions/source_editor_file_template_ext.js b/app/assets/javascripts/editor/extensions/source_editor_file_template_ext.js index 50b3ed54beed3c..ba4980896e5870 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_file_template_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_file_template_ext.js @@ -1,6 +1,10 @@ import { Position } from 'monaco-editor'; export class FileTemplateExtension { + static get extensionName() { + return 'FileTemplate'; + } + // eslint-disable-next-line class-methods-use-this provides() { return { diff --git a/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js b/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js index 106669432b102d..a16fe93026ee18 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_markdown_ext.js @@ -1,4 +1,8 @@ export class EditorMarkdownExtension { + static get extensionName() { + return 'EditorMarkdown'; + } + // eslint-disable-next-line class-methods-use-this provides() { return { diff --git a/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js b/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js index 9a1fcd333f39ec..9d53268c3407e9 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_markdown_livepreview_ext.js @@ -34,6 +34,10 @@ const setupDomElement = ({ injectToEl = null } = {}) => { }; export class EditorMarkdownPreviewExtension { + static get extensionName() { + return 'EditorMarkdownPreview'; + } + onSetup(instance, setupOptions) { this.preview = { el: undefined, diff --git a/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js b/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js index 2313b37d060f3f..4e8c11bac54401 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_webide_ext.js @@ -69,6 +69,10 @@ const updateInstanceDimensions = (instance) => { }; export class EditorWebIdeExtension { + static get extensionName() { + return 'EditorWebIde'; + } + /** * Set up the WebIDE extension for Source Editor * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance diff --git a/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js b/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js index 2cf71aa6fddba9..05ce617ca7c599 100644 --- a/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js +++ b/app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js @@ -15,6 +15,10 @@ import { parseDocument, Document, visit, isScalar, isCollection, isMap } from 'y import { findPair } from 'yaml/util'; export class YamlEditorExtension { + static get extensionName() { + return 'YamlEditor'; + } + /** * Extends the source editor with capabilities for yaml files. * diff --git a/app/assets/javascripts/editor/source_editor_extension.js b/app/assets/javascripts/editor/source_editor_extension.js index f6bc62a1c09366..6d47e1e22480c1 100644 --- a/app/assets/javascripts/editor/source_editor_extension.js +++ b/app/assets/javascripts/editor/source_editor_extension.js @@ -5,10 +5,10 @@ export default class EditorExtension { if (typeof definition !== 'function') { throw new Error(EDITOR_EXTENSION_DEFINITION_ERROR); } - this.name = definition.name; // both class- and fn-based extensions have a name this.setupOptions = setupOptions; // eslint-disable-next-line new-cap this.obj = new definition(); + this.extensionName = definition.extensionName || this.obj.extensionName; // both class- and fn-based extensions have a name } get api() { diff --git a/app/assets/javascripts/editor/source_editor_instance.js b/app/assets/javascripts/editor/source_editor_instance.js index c307a982b82cfd..8372a59964b542 100644 --- a/app/assets/javascripts/editor/source_editor_instance.js +++ b/app/assets/javascripts/editor/source_editor_instance.js @@ -13,7 +13,7 @@ * A Source Editor Extension * @typedef {Object} SourceEditorExtension * @property {Object} obj - * @property {string} name + * @property {string} extensionName * @property {Object} api */ @@ -43,12 +43,12 @@ const utils = { } }, - getStoredExtension: (extensionsStore, name) => { + getStoredExtension: (extensionsStore, extensionName) => { if (!extensionsStore) { logError(EDITOR_EXTENSION_STORE_IS_MISSING_ERROR); return undefined; } - return extensionsStore.get(name); + return extensionsStore.get(extensionName); }, }; @@ -129,7 +129,7 @@ export default class EditorInstance { } // Existing Extension Path - const existingExt = utils.getStoredExtension(extensionsStore, definition.name); + const existingExt = utils.getStoredExtension(extensionsStore, definition.extensionName); if (existingExt) { if (isEqual(extension.setupOptions, existingExt.setupOptions)) { return existingExt; @@ -156,14 +156,14 @@ export default class EditorInstance { * @param {Map} extensionsStore - The global registry for the extension instances */ registerExtension(extension, extensionsStore) { - const { name } = extension; + const { extensionName } = extension; const hasExtensionRegistered = - extensionsStore.has(name) && - isEqual(extension.setupOptions, extensionsStore.get(name).setupOptions); + extensionsStore.has(extensionName) && + isEqual(extension.setupOptions, extensionsStore.get(extensionName).setupOptions); if (hasExtensionRegistered) { return; } - extensionsStore.set(name, extension); + extensionsStore.set(extensionName, extension); const { obj: extensionObj } = extension; if (extensionObj.onUse) { extensionObj.onUse(this); @@ -175,7 +175,7 @@ export default class EditorInstance { * @param {SourceEditorExtension} extension - Instance of Source Editor extension */ registerExtensionMethods(extension) { - const { api, name } = extension; + const { api, extensionName } = extension; if (!api) { return; @@ -185,7 +185,7 @@ export default class EditorInstance { if (this[prop]) { logError(sprintf(EDITOR_EXTENSION_NAMING_CONFLICT_ERROR, { prop })); } else { - this.methods[prop] = name; + this.methods[prop] = extensionName; } }, this); } @@ -203,10 +203,10 @@ export default class EditorInstance { if (!extension) { throw new Error(EDITOR_EXTENSION_NOT_SPECIFIED_FOR_UNUSE_ERROR); } - const { name } = extension; - const existingExt = utils.getStoredExtension(extensionsStore, name); + const { extensionName } = extension; + const existingExt = utils.getStoredExtension(extensionsStore, extensionName); if (!existingExt) { - throw new Error(sprintf(EDITOR_EXTENSION_NOT_REGISTERED_ERROR, { name })); + throw new Error(sprintf(EDITOR_EXTENSION_NOT_REGISTERED_ERROR, { extensionName })); } const { obj: extensionObj } = existingExt; if (extensionObj.onBeforeUnuse) { @@ -223,12 +223,12 @@ export default class EditorInstance { * @param {SourceEditorExtension} extension - Instance of Source Editor extension to un-use */ unregisterExtensionMethods(extension) { - const { api, name } = extension; + const { api, extensionName } = extension; if (!api) { return; } Object.keys(api).forEach((method) => { - utils.removeExtFromMethod(method, name, this.methods); + utils.removeExtFromMethod(method, extensionName, this.methods); }); } diff --git a/spec/frontend/editor/helpers.js b/spec/frontend/editor/helpers.js index 902a1944187ac4..252d783ad6db0a 100644 --- a/spec/frontend/editor/helpers.js +++ b/spec/frontend/editor/helpers.js @@ -13,6 +13,10 @@ export const spyOnApi = (extension, spiesObj = {}) => { // Dummy Extensions export class SEClassExtension { + static get extensionName() { + return 'SEClassExtension'; + } + // eslint-disable-next-line class-methods-use-this provides() { return { @@ -24,6 +28,7 @@ export class SEClassExtension { export function SEFnExtension() { return { + extensionName: 'SEFnExtension', fnExtMethod: () => 'fn own method', provides: () => { return { @@ -35,6 +40,7 @@ export function SEFnExtension() { export const SEConstExt = () => { return { + extensionName: 'SEConstExt', provides: () => { return { constExtMethod: () => 'const own method', @@ -44,6 +50,9 @@ export const SEConstExt = () => { }; export class SEWithSetupExt { + static get extensionName() { + return 'SEWithSetupExt'; + } // eslint-disable-next-line class-methods-use-this onSetup(instance, setupOptions = {}) { if (setupOptions && !Array.isArray(setupOptions)) { @@ -72,6 +81,7 @@ export class SEWithSetupExt { export const conflictingExtensions = { WithInstanceExt: () => { return { + extensionName: 'WithInstanceExt', provides: () => { return { use: () => 'A conflict with instance', @@ -82,6 +92,7 @@ export const conflictingExtensions = { }, WithAnotherExt: () => { return { + extensionName: 'WithAnotherExt', provides: () => { return { shared: () => 'A conflict with extension', diff --git a/spec/frontend/editor/source_editor_extension_spec.js b/spec/frontend/editor/source_editor_extension_spec.js index de3f9da0aed348..c5fa795f3b74b3 100644 --- a/spec/frontend/editor/source_editor_extension_spec.js +++ b/spec/frontend/editor/source_editor_extension_spec.js @@ -40,7 +40,7 @@ describe('Editor Extension', () => { expect(extension).toEqual( expect.objectContaining({ - name: expectedName, + extensionName: expectedName, setupOptions, }), ); diff --git a/spec/frontend/editor/source_editor_instance_spec.js b/spec/frontend/editor/source_editor_instance_spec.js index f13c88346ba984..f9518743ef8c2d 100644 --- a/spec/frontend/editor/source_editor_instance_spec.js +++ b/spec/frontend/editor/source_editor_instance_spec.js @@ -34,6 +34,10 @@ describe('Source Editor Instance', () => { const fooFn = jest.fn(); const fooProp = 'foo'; class DummyExt { + // eslint-disable-next-line class-methods-use-this + get extensionName() { + return 'DummyExt'; + } // eslint-disable-next-line class-methods-use-this provides() { return { diff --git a/spec/frontend/ide/components/repo_editor_spec.js b/spec/frontend/ide/components/repo_editor_spec.js index 93bc3a0e11c49a..c957c64aa1086f 100644 --- a/spec/frontend/ide/components/repo_editor_spec.js +++ b/spec/frontend/ide/components/repo_editor_spec.js @@ -265,10 +265,10 @@ describe('RepoEditor', () => { it('installs the WebIDE extension', async () => { await createComponent(); expect(applyExtensionSpy).toHaveBeenCalled(); - const ideExtensionApi = extensionsStore.get('EditorWebIdeExtension').api; + const ideExtensionApi = extensionsStore.get('EditorWebIde').api; Reflect.ownKeys(ideExtensionApi).forEach((fn) => { expect(vm.editor[fn]).toBeDefined(); - expect(vm.editor.methods[fn]).toBe('EditorWebIdeExtension'); + expect(vm.editor.methods[fn]).toBe('EditorWebIde'); }); }); @@ -356,7 +356,7 @@ describe('RepoEditor', () => { let updateDimensionsSpy; beforeEach(async () => { await createComponent(); - const ext = extensionsStore.get('EditorWebIdeExtension'); + const ext = extensionsStore.get('EditorWebIde'); updateDimensionsSpy = jest.fn(); spyOnApi(ext, { updateDimensions: updateDimensionsSpy, @@ -430,7 +430,7 @@ describe('RepoEditor', () => { activeFile: dummyFile.markdown, }); - const ext = extensionsStore.get('EditorWebIdeExtension'); + const ext = extensionsStore.get('EditorWebIde'); updateDimensionsSpy = jest.fn(); spyOnApi(ext, { updateDimensions: updateDimensionsSpy, -- GitLab