Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#12] Fix undefined return of .modules if no monaco plugin is installed. #13

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[#12] Fix undefined return of .modules if no monaco plugin is installed. #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9991554
[#12] Fix undefined return of .modules if no monaco plugin is installed.
Gk0Wk Mar 26, 2022
ea9213d
Merge branch 'SmilyOrg:main' into main
Gk0Wk Apr 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion plugins/monaco-wikitext/plugin.info
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@
"source": "https://github.com/smilyorg/tw5-monaco",
"plugin-type": "plugin",
"parent-plugin": "$:/plugins/smilyorg/monaco",
"list": "readme license"
"list": "readme license",
"dependents": "$:/plugins/smilyorg/monaco"
}
73 changes: 31 additions & 42 deletions plugins/monaco/engine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@ Text editor engine based on a Monaco instance
/*jslint browser: true */
/*global $tw: false */
"use strict";

const HEIGHT_VALUE_TITLE = "$:/config/TextEditor/EditorHeight/Height";

/**
* Executes and returns the monaco loader based on the code contained
* in the provided tiddler.
* @param {string} title loader.js tiddler title
* @param {string} title loader.js tiddler title
* @returns context object containing `require` and `define` used to load monaco
*/
function getLoader(title) {
Expand All @@ -42,7 +42,7 @@ Text editor engine based on a Monaco instance
vs: "https://unpkg.com/[email protected]/min/vs"
}
});

// Quick hack to make the monaco loader implementation work,
// it might conflict with other plugins/code, but for vanilla TiddlyWiki
// it seems to work fine.
Expand All @@ -52,25 +52,15 @@ Text editor engine based on a Monaco instance
// as a result of this require and some also on-the-fly (for e.g. language support)
context.require(["vs/editor/editor.main"], function() {
window.monacoInit.dispatchEvent(new Event("ready"));

// Run all monaco plugins
const modules = $tw.modules.types["monaco"];
const req = Object.getOwnPropertyNames(modules);
if (req) {
if (Array.isArray(req)) {
for (const r of req) {
require(r);
}
} else {
require(req);
}
}
$tw.modules.forEachModuleOfType('monaco', function (title, exportModules) {
// Actually, no need to call require(title)
});
Comment on lines +55 to +57
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you're right. In that case this whole block can be deleted, right?

I copied this from CodeMirror's engine.js assuming it was needed, so I wonder why it was added there? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See boot.js in tiddlywiki:

$tw.modules.forEachModuleOfType = function(moduleType,callback) {
	var modules = $tw.modules.types[moduleType];
	$tw.utils.each(modules,function(element,title) {
		callback(title,$tw.modules.execute(title));
	});
};

this method has already executed each code of module tiddlers.

Only why CodeMirror adds these codes, I think maybe there is no auto-loading mechanism at the beginning btw, or maybe the codes themselves are unnecessary. The reason why the CodeMirror plugin does not have errors is that the CodeMirror plugin must contain at least one plugin.

I think we should do the same - using the tw-monaco plugin must be used to write tw in the first place, right?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response!

I initially tried to model it closer to CodeMirror with features being separate plugins, but then I realized it might just make it harder to install. So I ended up with the main monaco plugin and a monaco-wikitext plugin that adds wikitext-specific functionality.

We can go a little further and just merge all the monaco-wikitext functionality into the main monaco plugin as I'm not sure there is any need for it to be separate anyway. If people only want some parts of it that could be done via plugin configuration.

What do you think?

});
}

function MonacoEngine(options) {
options = options || {};

this.widget = options.widget;
this.wiki = this.widget.wiki;
this.value = options.value;
Expand All @@ -79,9 +69,9 @@ Text editor engine based on a Monaco instance
type: options.type,
value: options.value,
}

this.domNode = this.widget.document.createElement("div");

let className = "tw-monaco-editor-wrapper";
if (this.widget.editClass) {
className += " " + this.widget.editClass;
Expand All @@ -100,7 +90,7 @@ Text editor engine based on a Monaco instance
window.monacoInit.addEventListener("ready", this.initMonaco.bind(this))
}
}

MonacoEngine.prototype.initMonaco = function() {
if (this.editor) {
throw new Error("Monaco already initialized");
Expand All @@ -125,7 +115,7 @@ Text editor engine based on a Monaco instance

this.addObserver();
this.addActions();

// Apply initial text/type
if (this.initial) {
this.updateModel(this.initial.value, this.initial.type);
Expand All @@ -145,22 +135,22 @@ Text editor engine based on a Monaco instance
/**
* Adds mutation observers to detect when the editor node has detached from
* the browser DOM to dispose of the attached editor resources.
*
*
* If Monaco isn't explicitly disposed, it seems to lead to a memory leak of
* ~5MB every time a new editor is allocated. This can happen more often than
* expected (e.g. toggling preview or just going into and out of edit mode).
*
*
* This is a workaround of TiddlyWiki not having a widget destructor,
* see https://github.com/Jermolene/TiddlyWiki5/pull/2280
*
*
*/
MonacoEngine.prototype.addObserver = function() {
if (this.observer) this.removeObserver();
this.observer = new MutationObserver(this.onMutation.bind(this));
let node = this.parentNode;
while (node != null) {
if (node.classList && (
node.classList.contains("tc-reveal") ||
node.classList.contains("tc-reveal") ||
node.classList.contains("tc-tiddler-frame")
)) {
this.observer.observe(node.parentNode, {
Expand All @@ -170,7 +160,7 @@ Text editor engine based on a Monaco instance
node = node.parentNode;
}
}

/**
* Removes the mutation observer added by `addObserver()`
*/
Expand All @@ -179,7 +169,7 @@ Text editor engine based on a Monaco instance
this.observer.disconnect();
this.observer = null;
}

MonacoEngine.prototype.addActions = function() {

this.editor.addAction({
Expand Down Expand Up @@ -259,7 +249,7 @@ Text editor engine based on a Monaco instance
case 220: return this.monaco.KeyCode.Backslash;
case 189: return this.monaco.KeyCode.Minus;
case 187: return this.monaco.KeyCode.Equal;

case 0: return this.monaco.KeyCode.ContextMenu;
case 13: return this.monaco.KeyCode.Enter;
case 32: return this.monaco.KeyCode.Space;
Expand Down Expand Up @@ -408,7 +398,7 @@ Text editor engine based on a Monaco instance

/**
* Update the editor model with the provided text and language type.
*
*
* @param {string} text The text contents to set for the editor.
* @param {string} type The MIME type of the language to apply to the editor.
*/
Expand All @@ -434,33 +424,33 @@ Text editor engine based on a Monaco instance
// Only update the text if it's different as TiddlyWiki
// often sets text that was updated and we don't want
// to mess up the internal state of the editor (e.g. cursor position)
// if the text is the same.
// if the text is the same.
} else if (model.getValue() != text) {
model.setValue(text);
}
}

/*
Set the text of the engine if it doesn't currently have focus
*/
MonacoEngine.prototype.setText = function(text,type) {
this.updateModel(text, type);
};

/*
Update the DomNode with the new text
*/
MonacoEngine.prototype.updateDomNodeText = function(text) {
// TODO: not sure what this should do, doesn't seem to get called
};

/*
Get the text of the engine
*/
MonacoEngine.prototype.getText = function() {
return this.editor.getValue();
};

/*
Fix the height of textarea to fit content
*/
Expand All @@ -482,14 +472,14 @@ Text editor engine based on a Monaco instance
height,
});
};

/*
Focus the engine node
*/
MonacoEngine.prototype.focus = function() {
// TODO: if/when is this called and do we want to focus the editor?
}

/*
Create a blank structure representing a text operation
*/
Expand All @@ -509,7 +499,7 @@ Text editor engine based on a Monaco instance
operation.selection = operation.text.substring(operation.selStart,operation.selEnd);
return operation;
};

/*
Execute a text operation
*/
Expand All @@ -533,8 +523,7 @@ Text editor engine based on a Monaco instance
);
return model.getValue();
};

exports.MonacoEngine = $tw.browser ? MonacoEngine : require("$:/core/modules/editor/engines/simple.js").SimpleEngine;

})();

Loading