Merge branch '241023-editor-cleanup' into 'master'

Moved model down to instance level

See merge request gitlab-org/gitlab!41179

app/assets/javascripts/editor/editor_lite.js

@@ -9,11 +9,7 @@ import { EDITOR_LITE_INSTANCE_ERROR_NO_EL, URI_PREFIX } from './constants';
 
 export default class Editor {
   constructor(options = {}) {
-    this.editorEl = null;
-    this.blobContent = '';
-    this.blobPath = '';
     this.instances = [];
-    this.model = null;
     this.options = {
       extraEditorClassName: 'gl-editor-lite',
       ...defaultEditorOptions,
@@ -32,6 +28,17 @@ export default class Editor {
     monacoEditor.setTheme(theme ? themeName : DEFAULT_THEME);
   }
 
+  static updateModelLanguage(path, instance) {
+    if (!instance) return;
+    const model = instance.getModel();
+    const ext = `.${path.split('.').pop()}`;
+    const language = monacoLanguages
+      .getLanguages()
+      .find(lang => lang.extensions.indexOf(ext) !== -1);
+    const id = language ? language.id : 'plaintext';
+    monacoEditor.setModelLanguage(model, id);
+  }
+
   /**
    * Creates a monaco instance with the given options.
    *
@@ -51,19 +58,18 @@ export default class Editor {
     if (!el) {
       throw new Error(EDITOR_LITE_INSTANCE_ERROR_NO_EL);
     }
-    this.editorEl = el;
-    this.blobContent = blobContent;
-    this.blobPath = blobPath;
 
-    clearDomElement(this.editorEl);
+    clearDomElement(el);
 
     const uriFilePath = joinPaths(URI_PREFIX, blobGlobalId, blobPath);
 
-    const model = monacoEditor.createModel(this.blobContent, undefined, Uri.file(uriFilePath));
+    const model = monacoEditor.createModel(blobContent, undefined, Uri.file(uriFilePath));
 
-    monacoEditor.onDidCreateEditor(this.renderEditor.bind(this));
+    monacoEditor.onDidCreateEditor(() => {
+      delete el.dataset.editorLoading;
+    });
 
-    const instance = monacoEditor.create(this.editorEl, {
+    const instance = monacoEditor.create(el, {
       ...this.options,
       ...instanceOptions,
     });
@@ -73,13 +79,7 @@ export default class Editor {
       this.instances.splice(index, 1);
       model.dispose();
     });
-    instance.updateModelLanguage = path => this.updateModelLanguage(path);
-
-    // Reference to the model on the editor level will go away in
-    // https://gitlab.com/gitlab-org/gitlab/-/issues/241023
-    // After that, the references to the model will be routed through
-    // instance exclusively
-    this.model = model;
+    instance.updateModelLanguage = path => Editor.updateModelLanguage(path, instance);
 
     this.instances.push(instance);
     return instance;
@@ -89,21 +89,6 @@ export default class Editor {
     this.instances.forEach(instance => instance.dispose());
   }
 
-  renderEditor() {
-    delete this.editorEl.dataset.editorLoading;
-  }
-
-  updateModelLanguage(path) {
-    if (path === this.blobPath) return;
-    this.blobPath = path;
-    const ext = `.${path.split('.').pop()}`;
-    const language = monacoLanguages
-      .getLanguages()
-      .find(lang => lang.extensions.indexOf(ext) !== -1);
-    const id = language ? language.id : 'plaintext';
-    monacoEditor.setModelLanguage(this.model, id);
-  }
-
   use(exts = [], instance = null) {
     const extensions = Array.isArray(exts) ? exts : [exts];
     if (instance) {

spec/frontend/editor/editor_lite_spec.js

@@ -26,9 +26,7 @@ describe('Base editor', () => {
 
   it('initializes Editor with basic properties', () => {
     expect(editor).toBeDefined();
-    expect(editor.editorEl).toBe(null);
-    expect(editor.blobContent).toBe('');
-    expect(editor.blobPath).toBe('');
+    expect(editor.instances).toEqual([]);
   });
 
   it('removes `editor-loading` data attribute from the target DOM element', () => {
@@ -59,10 +57,6 @@ describe('Base editor', () => {
         editor.createInstance();
       }).toThrow(EDITOR_LITE_INSTANCE_ERROR_NO_EL);
 
-      expect(editor.editorEl).toBe(null);
-      expect(editor.blobContent).toBe('');
-      expect(editor.blobPath).toBe('');
-
       expect(modelSpy).not.toHaveBeenCalled();
       expect(instanceSpy).not.toHaveBeenCalled();
       expect(setModel).not.toHaveBeenCalled();
@@ -93,14 +87,14 @@ describe('Base editor', () => {
     });
 
     it('initializes instance with passed properties', () => {
+      const instanceOptions = {
+        foo: 'bar',
+      };
       editor.createInstance({
         el: editorEl,
-        blobContent,
-        blobPath,
+        ...instanceOptions,
       });
-      expect(editor.editorEl).toBe(editorEl);
-      expect(editor.blobContent).toBe(blobContent);
-      expect(editor.blobPath).toBe(blobPath);
+      expect(instanceSpy).toHaveBeenCalledWith(editorEl, expect.objectContaining(instanceOptions));
     });
 
     it('disposes instance when the editor is disposed', () => {
@@ -149,16 +143,26 @@ describe('Base editor', () => {
 
     it('can initialize several instances of the same editor', () => {
       editor.createInstance(inst1Args);
-      expect(editor.editorEl).toBe(editorEl1);
       expect(editor.instances).toHaveLength(1);
 
       editor.createInstance(inst2Args);
-      expect(editor.editorEl).toBe(editorEl2);
 
       expect(instanceSpy).toHaveBeenCalledTimes(2);
       expect(editor.instances).toHaveLength(2);
     });
 
+    it('sets independent models on independent instances', () => {
+      inst1 = editor.createInstance(inst1Args);
+      inst2 = editor.createInstance(inst2Args);
+
+      const model1 = inst1.getModel();
+      const model2 = inst2.getModel();
+
+      expect(model1).toBeDefined();
+      expect(model2).toBeDefined();
+      expect(model1).not.toEqual(model2);
+    });
+
     it('shares global editor options among all instances', () => {
       editor = new Editor({
         readOnly: true,
@@ -218,20 +222,20 @@ describe('Base editor', () => {
 
       const blobRenamedPath = 'test.js';
 
-      expect(editor.model.getLanguageIdentifier().language).toBe('markdown');
-      editor.updateModelLanguage(blobRenamedPath);
+      expect(instance.getModel().getLanguageIdentifier().language).toBe('markdown');
+      instance.updateModelLanguage(blobRenamedPath);
 
-      expect(editor.model.getLanguageIdentifier().language).toBe('javascript');
+      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(() => {});
 
-      editor.updateModelLanguage(blobRenamedPath);
+      instance.updateModelLanguage(blobRenamedPath);
 
       expect(spy).not.toHaveBeenCalled();
-      expect(editor.model.getLanguageIdentifier().language).toBe('plaintext');
+      expect(instance.getModel().getLanguageIdentifier().language).toBe('plaintext');
     });
   });
 
@@ -298,7 +302,6 @@ describe('Base editor', () => {
       };
       editor.use(FunctionExt);
       expect(instance.inst()).toEqual(editor.instances[0]);
-      expect(instance.mod()).toEqual(editor.model);
     });
 
     describe('multiple instances', () => {

spec/frontend/editor/editor_markdown_ext_spec.js

@@ -35,7 +35,7 @@ describe('Markdown Extension for Editor Lite', () => {
   });
 
   afterEach(() => {
-    editor.model.dispose();
+    instance.dispose();
     editorEl.remove();
   });