angular-ui/ui-codemirror

Options not working correctly due to way directive feeds them to CodeMirror, need to be ordered correctly

WhatFreshHellIsThis opened this issue · 9 comments

I'm having a wierd issue with linting not working and getting an error about it (Error: Required option 'getAnnotations' missing (lint addon)) and so far no normal or usual reason for this has been found related to CodeMirror itself so I'm looking farther afield.

I've confirmed some things with the CodeMirror author and still not explanation found, it should be working.

I'm guessing using this directive shouldn't affect this since everything else seems to work fine but just thought I would check to be certain.

Can you put a demo there ?

I found a problem and I think it's likely related to my original issue but not 100% certain, this is a definite issue however with the directive.

Turns out when you use the directive, the order of the configuration options matters if you want to see lint markers in the gutter but when you're not using the directive it doesn't matter at all:

Here's what I'm seeing, this is my markup:

 <div ui-codemirror="scriptEditorOptions" ui-refresh='refreshScriptEditor' ng-model="report.script"></div>

This is the minimum configuration that triggers the problem:

//this will not show lint markers in the gutter due to the option "lint: true" appearing before the option 
//"gutters"
 $scope.scriptEditorOptions = {
                lineNumbers: true,
                mode: 'javascript',
                lint: true,
                gutters: ['CodeMirror-lint-markers']
            };

This configuration will show the markers in the gutters, note that it's exactly the same except the lint and gutters lines have been transposed so that gutters comes first:

//this *will* show lint markers in the gutter
 $scope.scriptEditorOptions = {
                lineNumbers: true,
                mode: 'javascript',               
                gutters: ['CodeMirror-lint-markers'],
                lint: true
            };

Now that seems like a potential bug in CodeMirror right? However it is not because you can transpose those same two lines in the stock CodeMirror example page for lint and there is no issue, lint markers still show, I've tested it:

 <p><textarea id="code-js">

</textarea></p>

<script>
//Lint before gutters in non-directive mode, still works
  var editor = CodeMirror.fromTextArea(document.getElementById("code-js"), {
    lineNumbers: true,
    mode: "javascript",
    lint: true,
    gutters: ["CodeMirror-lint-markers"]
  });
</script>

So it seems that something in the directives updateOptions method or surrounding code is causing CodeMirror to get confused. I'm guessing this is the same reason why I'm having other issues as initially reported but this was the first direct evidence I can find that there is a difference between using CodeMirror with and without the Angular directive. I'll continue to dig.

Ok, did some more digging, this is definitely an issue with this directive and a serious one at that. Normally with CodeMirror the order the options are set in does not matter at all (I've confirmed for certain) but when using the directive it matters very much. For example if using the directive you put mode: 'javascript' after the lint: true option then it gives a nasty error, but when not using the directive this is perfectly valid.

Or (only when using the Angular directive) your config has the lint: true option specified before the gutters option then there is no warning display in the gutter at all whereas this is not an issue in non directive mode.

The problem appears to be that the setOption method used by the directive to feed the options one by one is not equivalent to whatever CodeMirror does itself when it reads in all the options at once when used in non Angular Directive mode.

I'm not sure if this is a bug with this directive or with CodeMirror itself. The issue I opened there is here: codemirror/codemirror5#2674

Stated by @marijnh over at CodeMirror repo in the issue I linked to:
"Indeed, setting options one at a time like this will cause problems for a few options. It is also super inefficient. I'd say that this is a bug in the Angular wrapper, not in CodeMirror."

As a workaround I have played with it repeatedly changing the options around until I found the combination that seems to work in the order they are fed to the CodeMirror component.

Hi @WhatFreshHellIsThis. I have been following your issue with interest. I agree with @marijnh it's clearly a naive approach to just set the options one by one.

If you want to stick to the setOption approach, at least follow the order of the properties in CodeMirror.defaults by doing a for/in loop over that, rather than depending on the object passed by the user. But it'd be better to build up an option object and pass that to the CodeMirror constructor, since CodeMirror can initialize more efficiently if it knows its full configuration upfront.

Thanks @marijnh 👍

@WhatFreshHellIsThis Here is the result of the last update I made

Looks good. I reordered the options in the Plunker so that they were in the order that broke for me here and it seems to work good! I also see you got ui-codemirror-opts working so we can change from ui-codemirror instead?