Page MenuHomePhabricator

ve.ui.MWAceEditorWidget breaks autoloading of text modes
Closed, ResolvedPublic1 Story Points


// TODO: Just use ace.require once T127643 is resolved
var require = ace.require || require;
widget.editor.getSession().setMode( 'ace/mode/' + ( require( 'ace/mode/' + lang ) ? lang : 'text' ) );

We are being too smart for our own good here...

Now that we have set the basePath for Ace, we can use auto loading, but the above strategy doesn't work with that methodology. The reason is that require, needs to have a module define itself first. Therefore, the module needs to be loading before code requires it. Since only few modes of the total amount of modes are preloaded by RL (ext.codeEditor.ace and ext.codeEditor.ace.modes), we are getting less than we could (because requiring an undefined module returns undefined).

Instead just use setMode directly and let ACE handle everything, including loading the resource so that it can define itself, requiring, and the setting of the language. It will also fallback to ace/mode/text on it's own.

widget.editor.getSession().setMode( 'ace/mode/' + lang );

If this is fixed, we probably don't need to load ext.codeEditor.ace.modes in the widget any longer either (or at all actually).


Related Gerrit Patches:
mediawiki/extensions/VisualEditor : masterEnable conditional loading of ACE language modes

Event Timeline

TheDJ created this task.Oct 18 2016, 3:04 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 18 2016, 3:04 PM

Change 316572 had a related patch set uploaded (by TheDJ):
Enable conditional loading of ACE language modes

TheDJ updated the task description. (Show Details)Oct 18 2016, 3:45 PM

Change 316572 merged by jenkins-bot:
Enable conditional loading of ACE language modes

TheDJ closed this task as Resolved.Oct 20 2016, 8:20 AM
TheDJ claimed this task.
TheDJ triaged this task as Low priority.
TheDJ removed a project: Patch-For-Review.
Jdforrester-WMF set the point value for this task to 1.