bouzidanas/streamlit-code-editor

Improve/add features to the Ace module such that current alterations/patches are not needed

Opened this issue · 0 comments

Core scripts (and extensions/plugins/themes) inside the ace-builds and react-ace node modules have been modified/patched to work with streamlit-code-editor. The problem is multifold.

  1. These modules should not have code specific to any application that uses said module.
  2. The functionality these changes provide should be transferable to future versions of the modules so that component doesnt break when any of the modified modules are updated.

The original reason for such modifications was that these modules were broken and/or are generally no longer maintained/updated.

Examples:

  • 'keyboard' was misspelled in some places the Ace keyboard shortcut extension which broke imports and prevented the extension from working.
  • Ace does not have the ability to set the default theme to something other than textmate and loading another theme when component is rerenders happens with a delay resulting in a flicker between themes.
  • Ace doesnt have theme removal functionality.
  • React-Ace lacks a way to get mode id from Syntax type object

Enhancement: General fixes and features should be added to Ace and React-Ace so that app-specific modifications are not needed.

For example, to add theme removal feature, a new attribute can be added to the theme module object containing other themes to remove after theme is loaded. The theme modules themselves can be defined with this attribute set:

Current solution:

ace.define("ace/theme/streamlit_light", ["require", "exports", "module", "ace/theme/streamlit_light.css", "ace/lib/dom"], function (require, exports, module) {
    exports.isDark = false;
    exports.cssClass = "ace-streamlit-light";
    exports.cssText = require("./streamlit_light.css");
    exports.$id = "ace/theme/streamlit_light";
    var dom = require("../lib/dom");
    dom.importCssString(exports.cssText, exports.cssClass, false);
    dom.removeElementById("ace-streamlit-dark");    //removes the style element containing the css text for the theme this theme replaces
});

And inside setTheme function:

function afterLoad(module) {
    if (_self.$themeId != theme)
        return cb && cb();
    if (!module || !module.cssClass)
        throw new Error("couldn't load module " + theme + " or it didn't call define");
    if (module.$id)
        _self.$themeId = module.$id;
    dom.importCssString(module.cssText, module.cssClass, _self.container);

    // The following IF/ELSE is a workaround for a theme replacement bug in
    // Streamlit component that uses ACE
    if (module.cssClass == "ace-streamlit-light") 
        dom.removeElementById("ace-streamlit-dark");
    else if (module.cssClass == "ace-streamlit-dark")
        dom.removeElementById("ace-streamlit-light");
    // ... //

Perhaps something like this would be better:

ace.define("ace/theme/streamlit_light", ["require", "exports", "module", "ace/theme/streamlit_light.css", "ace/lib/dom"], function (require, exports, module) {
    exports.isDark = false;
    exports.cssClass = "ace-streamlit-light";
    exports.cssText = require("./streamlit_light.css");
    exports.$id = "ace/theme/streamlit_light";
    exports.cssRemove = ["ace-streamlit-dark"]
    var dom = require("../lib/dom");
    dom.importCssString(exports.cssText, exports.cssClass, false);
   replaces
});

And inside setTheme function:

function afterLoad(module) {
    if (_self.$themeId != theme)
        return cb && cb();
    if (!module || !module.cssClass)
        throw new Error("couldn't load module " + theme + " or it didn't call define");
    if (module.$id)
        _self.$themeId = module.$id;
    dom.importCssString(module.cssText, module.cssClass, _self.container);
    if (module.cssRemove && module.cssRemove.length > 0) {
        module.cssRemove.forEach( function(cssClass) {
            dom.removeElementById(cssClass);
        });
    }
    // ... //

Obviously more consideration is needed.