From 6e41c79c94e225220f28ff6173b0d579535f36cd Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Thu, 4 Nov 2021 23:26:32 +0100 Subject: [PATCH 1/5] Source Editor Extension module While refactoring the Source Editor architecture it has been decided to split the monolythic Source Editor core into different modules. EditroExtension is one of them Changelog: changed --- app/assets/javascripts/editor/constants.js | 4 + .../editor/source_editor_extension.js | 17 ++++ locale/gitlab.pot | 3 + .../editor/source_editor_extension_spec.js | 99 +++++++++++++++++++ 4 files changed, 123 insertions(+) create mode 100644 app/assets/javascripts/editor/source_editor_extension.js create mode 100644 spec/frontend/editor/source_editor_extension_spec.js diff --git a/app/assets/javascripts/editor/constants.js b/app/assets/javascripts/editor/constants.js index d40d19000fb3..ffa2770a50af 100644 --- a/app/assets/javascripts/editor/constants.js +++ b/app/assets/javascripts/editor/constants.js @@ -20,6 +20,10 @@ export const EDITOR_TYPE_DIFF = 'vs.editor.IDiffEditor'; export const EDITOR_CODE_INSTANCE_FN = 'createInstance'; export const EDITOR_DIFF_INSTANCE_FN = 'createDiffInstance'; +export const EDITOR_EXTENSION_DEFINITION_ERROR = __( + 'Extension definition should be either a class or a function', +); + // // EXTENSIONS' CONSTANTS // diff --git a/app/assets/javascripts/editor/source_editor_extension.js b/app/assets/javascripts/editor/source_editor_extension.js new file mode 100644 index 000000000000..98a5883dfa06 --- /dev/null +++ b/app/assets/javascripts/editor/source_editor_extension.js @@ -0,0 +1,17 @@ +import { EDITOR_EXTENSION_DEFINITION_ERROR } from './constants'; + +export default class EditorExtension { + constructor({ definition, setupOptions } = {}) { + 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(); + } + + getApi() { + return this.obj.provides(); + } +} diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5c8c68900a2e..137f579ac29c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -14083,6 +14083,9 @@ msgstr "" msgid "Exported requirements" msgstr "" +msgid "Extension definition should be either a class or a function" +msgstr "" + msgid "External Classification Policy Authorization" msgstr "" diff --git a/spec/frontend/editor/source_editor_extension_spec.js b/spec/frontend/editor/source_editor_extension_spec.js new file mode 100644 index 000000000000..af164e9d684f --- /dev/null +++ b/spec/frontend/editor/source_editor_extension_spec.js @@ -0,0 +1,99 @@ +import EditorExtension from '~/editor/source_editor_extension'; +import { EDITOR_EXTENSION_DEFINITION_ERROR } from '~/editor/constants'; + +class MyClassExtension { + // eslint-disable-next-line class-methods-use-this + provides() { + return { + shared: () => 'extension', + classExtMethod: () => 'class own method', + }; + } +} + +function MyFnExtension() { + return { + fnExtMethod: () => 'fn own method', + provides: () => { + return { + shared: () => 'extension', + }; + }, + }; +} + +const MyConstExt = () => { + return { + provides: () => { + return { + shared: () => 'extension', + constExtMethod: () => 'const own method', + }; + }, + }; +}; + +describe('Editor Extension', () => { + const dummyObj = { foo: 'bar' }; + + it.each` + definition | setupOptions + ${undefined} | ${undefined} + ${undefined} | ${{}} + ${undefined} | ${dummyObj} + ${{}} | ${dummyObj} + ${dummyObj} | ${dummyObj} + `( + 'throws when definition = $definition and setupOptions = $setupOptions', + ({ definition, setupOptions }) => { + const constructExtension = () => new EditorExtension({ definition, setupOptions }); + expect(constructExtension).toThrowError(EDITOR_EXTENSION_DEFINITION_ERROR); + }, + ); + + it.each` + definition | setupOptions | expectedName + ${MyClassExtension} | ${undefined} | ${'MyClassExtension'} + ${MyClassExtension} | ${{}} | ${'MyClassExtension'} + ${MyClassExtension} | ${dummyObj} | ${'MyClassExtension'} + ${MyFnExtension} | ${undefined} | ${'MyFnExtension'} + ${MyFnExtension} | ${{}} | ${'MyFnExtension'} + ${MyFnExtension} | ${dummyObj} | ${'MyFnExtension'} + ${MyConstExt} | ${undefined} | ${'MyConstExt'} + ${MyConstExt} | ${{}} | ${'MyConstExt'} + ${MyConstExt} | ${dummyObj} | ${'MyConstExt'} + `( + 'correctly creates extension for definition = $definition and setupOptions = $setupOptions', + ({ definition, setupOptions, expectedName }) => { + const extension = new EditorExtension({ definition, setupOptions }); + // eslint-disable-next-line new-cap + const constructedDefinition = new definition(); + + expect(extension).toEqual( + expect.objectContaining({ + name: expectedName, + setupOptions, + }), + ); + expect(extension.obj.constructor.prototype).toBe(constructedDefinition.constructor.prototype); + }, + ); + + describe('getApi', () => { + it.each` + definition | expectedKeys + ${MyClassExtension} | ${['shared', 'classExtMethod']} + ${MyFnExtension} | ${['shared']} + ${MyConstExt} | ${['shared', 'constExtMethod']} + `('correctly returns API for $definition', ({ definition, expectedKeys }) => { + const extension = new EditorExtension({ definition }); + const expectedApi = {}; + + expectedKeys.forEach((key) => { + expectedApi[key] = expect.any(Function); + }); + + expect(extension.getApi()).toEqual(expect.objectContaining(expectedApi)); + }); + }); +}); From 72d2ada73bd864fcbf9b4abc2cfb518d8dfbf8b5 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Fri, 5 Nov 2021 17:02:08 +0100 Subject: [PATCH 2/5] Added the life-cycle interface of an extension --- .../editor/source_editor_extension.js | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/app/assets/javascripts/editor/source_editor_extension.js b/app/assets/javascripts/editor/source_editor_extension.js index 98a5883dfa06..1e66d3eca18c 100644 --- a/app/assets/javascripts/editor/source_editor_extension.js +++ b/app/assets/javascripts/editor/source_editor_extension.js @@ -14,4 +14,52 @@ export default class EditorExtension { getApi() { return this.obj.provides(); } + + /** + * THE LIFE-CYCLE CALLBACKS + */ + + /** + * Is called before the extension gets used by an instance, + * Use `onSetup` to setup Monaco directly: + * actions, keystrokes, update options, etc. + * Is called only once before the extension gets registered + * + * @param { Object } [options] The setupOptions object + * @param { Object } [instance] The Source Editor instance + */ + // eslint-disable-next-line class-methods-use-this,no-unused-vars + onSetup(options, instance) {} + + /** + * The first thing called after the extension is + * registered and used by an instance. + * Is called every time the extension is applied + * + * @param { Object } [instance] The Source Editor instance + */ + // eslint-disable-next-line class-methods-use-this,no-unused-vars + onUse(instance) {} + + /** + * Is called before un-using an extension. Can be used for time-critical + * actions like cleanup, reverting visual changes, and other user-facing + * updates. + * + * @param { Object } [instance] The Source Editor instance + */ + // eslint-disable-next-line class-methods-use-this,no-unused-vars + onBeforeUnuse(instance) {} + + /** + * Is called right after an extension is removed from an instance (un-used) + * Can be used for non time-critical tasks like cleanup on the Monaco level + * (removing actions, keystrokes, etc.). + * onUnuse() will be executed during the browser's idle period + * (https://developer.mozilla.org/en-US/docs/Web/API/Window/requestIdleCallback) + * + * @param { Object } [instance] The Source Editor instance + */ + // eslint-disable-next-line class-methods-use-this,no-unused-vars + onUnuse(instance) {} } From dde929c79062906ce524b178d133dd7f25e0a2d4 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Mon, 8 Nov 2021 14:18:11 +0100 Subject: [PATCH 3/5] API as a getter As per review suggestion --- .../javascripts/editor/source_editor_extension.js | 2 +- .../frontend/editor/source_editor_extension_spec.js | 13 +++++-------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/editor/source_editor_extension.js b/app/assets/javascripts/editor/source_editor_extension.js index 1e66d3eca18c..56491f6620bd 100644 --- a/app/assets/javascripts/editor/source_editor_extension.js +++ b/app/assets/javascripts/editor/source_editor_extension.js @@ -11,7 +11,7 @@ export default class EditorExtension { this.obj = new definition(); } - getApi() { + get api() { return this.obj.provides(); } diff --git a/spec/frontend/editor/source_editor_extension_spec.js b/spec/frontend/editor/source_editor_extension_spec.js index af164e9d684f..ebeeae7e42f6 100644 --- a/spec/frontend/editor/source_editor_extension_spec.js +++ b/spec/frontend/editor/source_editor_extension_spec.js @@ -79,7 +79,7 @@ describe('Editor Extension', () => { }, ); - describe('getApi', () => { + describe('api', () => { it.each` definition | expectedKeys ${MyClassExtension} | ${['shared', 'classExtMethod']} @@ -87,13 +87,10 @@ describe('Editor Extension', () => { ${MyConstExt} | ${['shared', 'constExtMethod']} `('correctly returns API for $definition', ({ definition, expectedKeys }) => { const extension = new EditorExtension({ definition }); - const expectedApi = {}; - - expectedKeys.forEach((key) => { - expectedApi[key] = expect.any(Function); - }); - - expect(extension.getApi()).toEqual(expect.objectContaining(expectedApi)); + const expectedApi = Object.fromEntries( + expectedKeys.map((key) => [key, expect.any(Function)]), + ); + expect(extension.api).toEqual(expect.objectContaining(expectedApi)); }); }); }); From 9316fe54887ccb94514ae7d13dbe0013312be8f9 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Tue, 9 Nov 2021 10:03:36 +0100 Subject: [PATCH 4/5] Created an example extension for the end users --- .../example_source_editor_extension.js | 116 ++++++++++++++++++ .../editor/source_editor_extension.js | 48 -------- 2 files changed, 116 insertions(+), 48 deletions(-) create mode 100644 app/assets/javascripts/editor/extensions/example_source_editor_extension.js diff --git a/app/assets/javascripts/editor/extensions/example_source_editor_extension.js b/app/assets/javascripts/editor/extensions/example_source_editor_extension.js new file mode 100644 index 000000000000..119a2aea9eb2 --- /dev/null +++ b/app/assets/javascripts/editor/extensions/example_source_editor_extension.js @@ -0,0 +1,116 @@ +// THIS IS AN EXAMPLE +// +// This file contains a basic documented example of the Source Editor extensions' +// API for your convenience. You can copy/paste it into your own file +// and adjust as you see fit +// + +export class MyFancyExtension { + /** + * THE LIFE-CYCLE CALLBACKS + */ + + /** + * Is called before the extension gets used by an instance, + * Use `onSetup` to setup Monaco directly: + * actions, keystrokes, update options, etc. + * Is called only once before the extension gets registered + * + * @param { Object } [setupOptions] The setupOptions object + * @param { Object } [instance] The Source Editor instance + */ + // eslint-disable-next-line class-methods-use-this,no-unused-vars + onSetup(setupOptions, instance) {} + + /** + * The first thing called after the extension is + * registered and used by an instance. + * Is called every time the extension is applied + * + * @param { Object } [instance] The Source Editor instance + */ + // eslint-disable-next-line class-methods-use-this,no-unused-vars + onUse(instance) {} + + /** + * Is called before un-using an extension. Can be used for time-critical + * actions like cleanup, reverting visual changes, and other user-facing + * updates. + * + * @param { Object } [instance] The Source Editor instance + */ + // eslint-disable-next-line class-methods-use-this,no-unused-vars + onBeforeUnuse(instance) {} + + /** + * Is called right after an extension is removed from an instance (un-used) + * Can be used for non time-critical tasks like cleanup on the Monaco level + * (removing actions, keystrokes, etc.). + * onUnuse() will be executed during the browser's idle period + * (https://developer.mozilla.org/en-US/docs/Web/API/Window/requestIdleCallback) + * + * @param { Object } [instance] The Source Editor instance + */ + // eslint-disable-next-line class-methods-use-this,no-unused-vars + onUnuse(instance) {} + + /** + * The public API of the extension: these are the methods that will be exposed + * to the end user + * @returns {Object} + */ + provides() { + return { + basic: () => { + // The most basic method not depending on anything + // Use: instance.basic(); + // eslint-disable-next-line @gitlab/require-i18n-strings + return 'Foo Bar'; + }, + basicWithProp: () => { + // The methods with access to the props of the extension. + // The props can be either hardcoded (for example in `onSetup`), or + // can be dynamically passed as part of `setupOptions` object when + // using the extension. + // Use: instance.use({ definition: MyFancyExtension, setupOptions: { foo: 'bar' }}); + return this.foo; + }, + basicWithPropsAsList: (prop1, prop2) => { + // Just a simple method with local props + // The props are passed as usually. + // Use: instance.basicWithPropsAsList(prop1, prop2); + // eslint-disable-next-line @gitlab/require-i18n-strings + return `The prop1 is ${prop1}; the prop2 is ${prop2}`; + }, + basicWithInstance: (instance) => { + // The method accessing the instance methods: either own or provided + // by previously-registered extensions + // `instance` is always supplied to all methods in provides() as THE LAST + // argument. + // You don't need to explicitly pass instance to this method: + // Use: instance.basicWithInstance(); + // eslint-disable-next-line @gitlab/require-i18n-strings + return `We have access to the whole Instance! ${instance.alpha()}`; + }, + advancedWithInstanceAndProps: ({ author, book } = {}, firstname, lastname, instance) => { + // Advanced method where + // { author, book } — are the props passed as an object + // prop1, prop2 — are the props passed as simple list + // instance — is automatically supplied, no need to pass it to + // the method explicitly + // Use: instance.advancedWithInstanceAndProps( + // { + // author: 'Franz Kafka', + // book: 'The Transformation' + // }, + // 'Franz', + // 'Kafka' + // ); + return ` +The author is ${author}; the book is ${book} +The author's name is ${firstname}; the last name is ${lastname} +We have access to the whole Instance! For example, 'instance.alpha()': ${instance.alpha()}`; + }, + }; + } +} diff --git a/app/assets/javascripts/editor/source_editor_extension.js b/app/assets/javascripts/editor/source_editor_extension.js index 56491f6620bd..664bcabcf452 100644 --- a/app/assets/javascripts/editor/source_editor_extension.js +++ b/app/assets/javascripts/editor/source_editor_extension.js @@ -14,52 +14,4 @@ export default class EditorExtension { get api() { return this.obj.provides(); } - - /** - * THE LIFE-CYCLE CALLBACKS - */ - - /** - * Is called before the extension gets used by an instance, - * Use `onSetup` to setup Monaco directly: - * actions, keystrokes, update options, etc. - * Is called only once before the extension gets registered - * - * @param { Object } [options] The setupOptions object - * @param { Object } [instance] The Source Editor instance - */ - // eslint-disable-next-line class-methods-use-this,no-unused-vars - onSetup(options, instance) {} - - /** - * The first thing called after the extension is - * registered and used by an instance. - * Is called every time the extension is applied - * - * @param { Object } [instance] The Source Editor instance - */ - // eslint-disable-next-line class-methods-use-this,no-unused-vars - onUse(instance) {} - - /** - * Is called before un-using an extension. Can be used for time-critical - * actions like cleanup, reverting visual changes, and other user-facing - * updates. - * - * @param { Object } [instance] The Source Editor instance - */ - // eslint-disable-next-line class-methods-use-this,no-unused-vars - onBeforeUnuse(instance) {} - - /** - * Is called right after an extension is removed from an instance (un-used) - * Can be used for non time-critical tasks like cleanup on the Monaco level - * (removing actions, keystrokes, etc.). - * onUnuse() will be executed during the browser's idle period - * (https://developer.mozilla.org/en-US/docs/Web/API/Window/requestIdleCallback) - * - * @param { Object } [instance] The Source Editor instance - */ - // eslint-disable-next-line class-methods-use-this,no-unused-vars - onUnuse(instance) {} } From 597d62d29c57b74427102a9c303050b10166d38f Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Wed, 10 Nov 2021 10:13:08 +0100 Subject: [PATCH 5/5] Contextualised translations for SE --- app/assets/javascripts/editor/constants.js | 14 +++++++------- locale/gitlab.pot | 18 +++++++++--------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/editor/constants.js b/app/assets/javascripts/editor/constants.js index ffa2770a50af..d44bfdfb966e 100644 --- a/app/assets/javascripts/editor/constants.js +++ b/app/assets/javascripts/editor/constants.js @@ -1,15 +1,15 @@ import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants'; -import { __ } from '~/locale'; +import { s__ } from '~/locale'; -export const SOURCE_EDITOR_INSTANCE_ERROR_NO_EL = __( - '"el" parameter is required for createInstance()', +export const SOURCE_EDITOR_INSTANCE_ERROR_NO_EL = s__( + 'SourceEditor|"el" parameter is required for createInstance()', ); export const URI_PREFIX = 'gitlab'; export const CONTENT_UPDATE_DEBOUNCE = DEFAULT_DEBOUNCE_AND_THROTTLE_MS; -export const ERROR_INSTANCE_REQUIRED_FOR_EXTENSION = __( - 'Source Editor instance is required to set up an extension.', +export const ERROR_INSTANCE_REQUIRED_FOR_EXTENSION = s__( + 'SourceEditor|Source Editor instance is required to set up an extension.', ); export const EDITOR_READY_EVENT = 'editor-ready'; @@ -20,8 +20,8 @@ export const EDITOR_TYPE_DIFF = 'vs.editor.IDiffEditor'; export const EDITOR_CODE_INSTANCE_FN = 'createInstance'; export const EDITOR_DIFF_INSTANCE_FN = 'createDiffInstance'; -export const EDITOR_EXTENSION_DEFINITION_ERROR = __( - 'Extension definition should be either a class or a function', +export const EDITOR_EXTENSION_DEFINITION_ERROR = s__( + 'SourceEditor|Extension definition should be either a class or a function', ); // diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 137f579ac29c..9b837cf8893e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -70,9 +70,6 @@ msgstr "" msgid "\"%{repository_name}\" size (%{repository_size}) is larger than the limit of %{limit}." msgstr "" -msgid "\"el\" parameter is required for createInstance()" -msgstr "" - msgid "#%{issueIid} (closed)" msgstr "" @@ -14083,9 +14080,6 @@ msgstr "" msgid "Exported requirements" msgstr "" -msgid "Extension definition should be either a class or a function" -msgstr "" - msgid "External Classification Policy Authorization" msgstr "" @@ -32568,9 +32562,6 @@ msgstr "" msgid "Source Branch" msgstr "" -msgid "Source Editor instance is required to set up an extension." -msgstr "" - msgid "Source IP" msgstr "" @@ -32589,6 +32580,15 @@ msgstr "" msgid "Source project cannot be found." msgstr "" +msgid "SourceEditor|\"el\" parameter is required for createInstance()" +msgstr "" + +msgid "SourceEditor|Extension definition should be either a class or a function" +msgstr "" + +msgid "SourceEditor|Source Editor instance is required to set up an extension." +msgstr "" + msgid "Sourcegraph" msgstr ""