Closed blittle closed 6 years ago
@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.
@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!
@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.
@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.
Thank you! I'll give it a shot and let you know.
On Sun, Jan 28, 2018 at 4:34 PM, Sal Niro notifications@github.com 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 https://github.com/scniro/react-codemirror2/issues/49#issuecomment-361106841, or mute the thread https://github.com/notifications/unsubscribe-auth/ABfoleN2cg6Yh_HJUWuHMvmAG8y97EIfks5tPQQGgaJpZM4RtwsI .
-- Bret Little Cell: 801-477-7618
@blittle Hey any good news on this one? Looking to close it out soon 😸
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 notifications@github.com 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 https://github.com/scniro/react-codemirror2/issues/49#issuecomment-362456902, or mute the thread https://github.com/notifications/unsubscribe-auth/ABfolaEPT6z6SruKTCM_FSjawj9uUvvhks5tQmQlgaJpZM4RtwsI .
-- Bret Little Cell: 801-477-7618
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:
Will not render lint warnings in the gutter. It works by instead doing:
I think the problem is two parts:
react-codemirror2
iterates in setting options: https://github.com/scniro/react-codemirror2/blob/master/src/index.tsx#L296LintState
when thelint: true
option is set. In that constructor, thehasGutter
flag is defined as false because thegutters
option has yet to be set: https://github.com/codemirror/CodeMirror/blob/master/addon/lint/lint.js#L62So, 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 inreact-codemirror2