Issue with the order of options

[blittle]

First of all, thank you for your work on this project. Especially given that react-codemirror appears abandoned.

I encountered an odd bug with the lint plugin. Specifically, I discovered that the order of options passed the CodeMirror component matter.

For example:

<CodeMirror options={{
  lint: true,
  gutters: ["CodeMirror-lint-markers"]  
}} />

Will not render lint warnings in the gutter. It works by instead doing:

<CodeMirror options={{
  gutters: ["CodeMirror-lint-markers"],
  lint: true
}} />

I think the problem is two parts:

1. react-codemirror2 iterates in setting options: https://github.com/scniro/react-codemirror2/blob/master/src/index.tsx#L296
2. Codemirror immediately invokes the constructor of the LintState when the lint: true option is set. In that constructor, the hasGutter flag is defined as false because the gutters option has yet to be set: https://github.com/codemirror/CodeMirror/blob/master/addon/lint/lint.js#L62

So, we could either try and get CodeMirror to not immediately invoke the constructor, or we could some how change react-codemirror2 to not iterate in setting options, but do it all at once.

Note: this is not a problem of react-codemirror, but only in react-codemirror2

[scniro]

@blittle Thank you so much for opening this up and doing most of the research on your own. This is totally valid, and now that I think about it, I'm surprised setting the options iteratively didn't jump out to me as a red flag earlier. I can definitely get a fix into this soon and will keep you posted with my findings. I hope your workaround is okay for now.

[blittle]

@scniro thank you for the quick response and your maintenance of this project. If I can be of help in building a fix, please let me know!

[scniro]

@blittle So I dug into this a little bit more and believe to have pinned the issue. Actually, I found a strikingly similar issue in a now older AnguarJS 1.x directive wrapper that suffered the same issue seen here. What really jumped out was the codemirror author's comment.

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.

Since this wrapper is responding to new options passed via props, I think it's best to keep this iterative approach, since we do indeed want to keep the living instance, but doing so merged with the defaults so the order is always correct, no matter what the user passed.

I have this working locally, and plan to ship it with a 4.x release that had various other chores that were lower priority. As soon as I get my changelog written these will be live, so expect to be able to pull the new package this weekend.

[scniro]

@blittle This should be fixed with the 4.0.0 release. If you find the time, could you try to reproduce this error with the order of options in the example as when you first posted this? If all looks good I'll close this out.

[blittle]

Thank you! I'll give it a shot and let you know.

…

On Sun, Jan 28, 2018 at 4:34 PM, Sal Niro ***@***.***> wrote: @blittle <https://github.com/blittle> This should be fixed with the 4.0.0 <https://github.com/scniro/react-codemirror2/releases/tag/4.0.0> release. If you find the time, could you try to reproduce this error with the order of options in the example as when you first posted this? If all looks good I'll close this out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABfoleN2cg6Yh_HJUWuHMvmAG8y97EIfks5tPQQGgaJpZM4RtwsI> .

-- *Bret Little* Cell: 801-477-7618

[scniro]

@blittle Hey any good news on this one? Looking to close it out soon 😸

[blittle]

Sorry for the delay. I'll get back to you within the next 12 hours.

…

On Thu, Feb 1, 2018 at 6:25 PM, Sal Niro ***@***.***> wrote: @blittle <https://github.com/blittle> Hey any good news on this one? Looking to close it out soon 😸 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABfolaEPT6z6SruKTCM_FSjawj9uUvvhks5tQmQlgaJpZM4RtwsI> .

-- *Bret Little* Cell: 801-477-7618