New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code mirror value doesn't update with state change in v1.0.0 #106

Open
MrSauceman opened this Issue May 15, 2017 · 25 comments

Comments

Projects
None yet
@MrSauceman

MrSauceman commented May 15, 2017

Changing props.value does not re-render code mirror. This is a new issue in v1.0.0, previously this worked. Changing props.value via the onChange event works fine, but programmatically changing props.value does not call a re-render.

This can be reproduced with the following code.

class Editor extends Component {
    constructor() {
        super();
        this.state = { value: 'abc' };
    }

    render() {
        const { value } = this.state;
        console.log(value);
        return (
            <div>
                <CodeMirror value={value} />
                <button onClick={() => this.setState({ value: 'def' })}>Click to change value</button>
            </div>
        );
    }
}

This appears like it may have something to do with this change listed in the history file: fixed; Only updates the CodeMirror value if props.value has changed.

@marcioaffonso

This comment has been minimized.

Show comment
Hide comment
@marcioaffonso

marcioaffonso May 15, 2017

Same issue here, @MrSauceman, do you have a workaround?

marcioaffonso commented May 15, 2017

Same issue here, @MrSauceman, do you have a workaround?

skidding added a commit to skidding/react-codemirror that referenced this issue May 16, 2017

@skidding skidding referenced a pull request that will close this issue May 16, 2017

Open

No longer debounce componentWillReceiveProps #106 #107

@skidding

This comment has been minimized.

Show comment
Hide comment
@skidding

skidding May 16, 2017

Contributor

Created a PR to fix this #107. I've commented on this issue two days ago:

I think this revealed another bug. In my case the Codemirror is no longer updated even when props change. I'm pretty sure it's because componentWillReceiveProps is debounced, which results in this.props being equal to nextProps when the lifecycle method is called in the next loop.

@JedWatson what do you think about no longer debouncing componentWillReceiveProps? We could make it optional, but I wouldn't bother since I believe it is not compatible with the React lifecycle model. If performance is the concern then let's leave the debouncing up to the user.

This is the PR that that introduced the debouncing: #35. Maybe @alexdmiller has more insight into this issue.

Contributor

skidding commented May 16, 2017

Created a PR to fix this #107. I've commented on this issue two days ago:

I think this revealed another bug. In my case the Codemirror is no longer updated even when props change. I'm pretty sure it's because componentWillReceiveProps is debounced, which results in this.props being equal to nextProps when the lifecycle method is called in the next loop.

@JedWatson what do you think about no longer debouncing componentWillReceiveProps? We could make it optional, but I wouldn't bother since I believe it is not compatible with the React lifecycle model. If performance is the concern then let's leave the debouncing up to the user.

This is the PR that that introduced the debouncing: #35. Maybe @alexdmiller has more insight into this issue.

@skidding

This comment has been minimized.

Show comment
Hide comment
@skidding

skidding May 16, 2017

Contributor

PS. Try using referencing my branch in your package.json (temporarily!) to see it solves your issues as well:

"react-codemirror": "git://github.com/skidding/react-codemirror.git#106-fix-update",

Update: You might want to use https://github.com/scniro/react-codemirror2

Contributor

skidding commented May 16, 2017

PS. Try using referencing my branch in your package.json (temporarily!) to see it solves your issues as well:

"react-codemirror": "git://github.com/skidding/react-codemirror.git#106-fix-update",

Update: You might want to use https://github.com/scniro/react-codemirror2

@marcioaffonso

This comment has been minimized.

Show comment
Hide comment
@marcioaffonso

marcioaffonso commented May 16, 2017

It works @skidding!

@skidding

This comment has been minimized.

Show comment
Hide comment
@skidding

skidding May 19, 2017

Contributor

Published my fork under @skidding/react-codemirror until #107 gets merged or resolved otherwise.

Update: You might want to use https://github.com/scniro/react-codemirror2

Contributor

skidding commented May 19, 2017

Published my fork under @skidding/react-codemirror until #107 gets merged or resolved otherwise.

Update: You might want to use https://github.com/scniro/react-codemirror2

@scniro

This comment has been minimized.

Show comment
Hide comment
@scniro

scniro May 25, 2017

+1 whats going on with this? Merging the PR fix would be soooooper

scniro commented May 25, 2017

+1 whats going on with this? Merging the PR fix would be soooooper

@rocktavious

This comment has been minimized.

Show comment
Hide comment
@rocktavious

rocktavious Jun 1, 2017

Bump! I'm having this issue too!

rocktavious commented Jun 1, 2017

Bump! I'm having this issue too!

@scniro

This comment has been minimized.

Show comment
Hide comment
@scniro

scniro Jun 1, 2017

For anyone who's still hung up on this or considering another editor, I'd highly recommend giving the raw codemirror library a look to use directly. I'm doing so with a tiny component wrapper I rolled and it's very straightforward. One less dep too (lodash as well)...

scniro commented Jun 1, 2017

For anyone who's still hung up on this or considering another editor, I'd highly recommend giving the raw codemirror library a look to use directly. I'm doing so with a tiny component wrapper I rolled and it's very straightforward. One less dep too (lodash as well)...

@MrSauceman

This comment has been minimized.

Show comment
Hide comment
@MrSauceman

MrSauceman Jun 1, 2017

@scniro care to share your wrapper?

MrSauceman commented Jun 1, 2017

@scniro care to share your wrapper?

@scniro

This comment has been minimized.

Show comment
Hide comment
@scniro

scniro Jun 1, 2017

@MrSauceman Of course. It's pretty bare bones, but here is what does the trick for me with some sample options...

wrapper

import React from 'react';
let codemirror = require('codemirror');

export default class CodeMirror extends React.Component {

  componentDidMount() {

    this.editor = codemirror(this.ref);
    this.editor.on('change', () => this.props.onChange(this.editor.getValue()));
  }

  componentWillReceiveProps(nextProps) {

    Object.keys(nextProps.options || {}).forEach(key => this.editor.setOption(key, nextProps.options[key]));
    this.editor.setValue(nextProps.value || '');
  }

  render() {
    return (
      <div ref={(self) => this.ref = self}/>
    )
  }
}

usage

<CodeMirror 
  value='foo' 
  options={{theme: 'material', viewportMargin: Infinity}} 
  onChange={value => {console.log(value); }} />

where value and options can be passed as a living prop from the parent component

scniro commented Jun 1, 2017

@MrSauceman Of course. It's pretty bare bones, but here is what does the trick for me with some sample options...

wrapper

import React from 'react';
let codemirror = require('codemirror');

export default class CodeMirror extends React.Component {

  componentDidMount() {

    this.editor = codemirror(this.ref);
    this.editor.on('change', () => this.props.onChange(this.editor.getValue()));
  }

  componentWillReceiveProps(nextProps) {

    Object.keys(nextProps.options || {}).forEach(key => this.editor.setOption(key, nextProps.options[key]));
    this.editor.setValue(nextProps.value || '');
  }

  render() {
    return (
      <div ref={(self) => this.ref = self}/>
    )
  }
}

usage

<CodeMirror 
  value='foo' 
  options={{theme: 'material', viewportMargin: Infinity}} 
  onChange={value => {console.log(value); }} />

where value and options can be passed as a living prop from the parent component

@scniro

This comment has been minimized.

Show comment
Hide comment
@scniro

scniro Jun 1, 2017

Also, should anyone find this useful I threw up a super quick package on npm => react-codemirror2. If others find this useful I'll be happy to maintain it moving forward and build it up however we see fit.

scniro commented Jun 1, 2017

Also, should anyone find this useful I threw up a super quick package on npm => react-codemirror2. If others find this useful I'll be happy to maintain it moving forward and build it up however we see fit.

@MartinHaeusler

This comment has been minimized.

Show comment
Hide comment
@MartinHaeusler

MartinHaeusler Jun 2, 2017

I think I ran into this issue as well. I'm using redux, so I can monitor which values are present in the application state. The codemirror editor clearly does not reflect the value property that was given to it. However, I am unable to pin down the cases when this bug occurs; in some regions of my UI it works as expected, in others the editor doesn't update...

MartinHaeusler commented Jun 2, 2017

I think I ran into this issue as well. I'm using redux, so I can monitor which values are present in the application state. The codemirror editor clearly does not reflect the value property that was given to it. However, I am unable to pin down the cases when this bug occurs; in some regions of my UI it works as expected, in others the editor doesn't update...

@arjanfrans

This comment has been minimized.

Show comment
Hide comment
@arjanfrans

arjanfrans Jun 12, 2017

I am having the same issue, this makes the library unusable for me. I downgraded for now.

arjanfrans commented Jun 12, 2017

I am having the same issue, this makes the library unusable for me. I downgraded for now.

@alexduf

This comment has been minimized.

Show comment
Hide comment
@alexduf

alexduf Jun 12, 2017

FWIW I tried it on a fresh project and couldn't get that feature to work. I switched to Ace editor which does the trick for me.

I'm not putting that comment to be dismissive or aggressive but as someone who doesn't know these libraries very well it took me a while to find out so here's a link: https://github.com/securingsincity/react-ace

alexduf commented Jun 12, 2017

FWIW I tried it on a fresh project and couldn't get that feature to work. I switched to Ace editor which does the trick for me.

I'm not putting that comment to be dismissive or aggressive but as someone who doesn't know these libraries very well it took me a while to find out so here's a link: https://github.com/securingsincity/react-ace

@stoplion

This comment has been minimized.

Show comment
Hide comment
@stoplion

stoplion Jun 19, 2017

would be awesome if this could be merged soon

stoplion commented Jun 19, 2017

would be awesome if this could be merged soon

@ympadilha

This comment has been minimized.

Show comment
Hide comment
@ympadilha

ympadilha Jun 24, 2017

I'm having this issue as well, gradly I've found this post! I'll try the solutions here mentioned but it would be really awesome to get this merged soon!

ympadilha commented Jun 24, 2017

I'm having this issue as well, gradly I've found this post! I'll try the solutions here mentioned but it would be really awesome to get this merged soon!

@inoas

This comment has been minimized.

Show comment
Hide comment
@inoas

inoas Jun 29, 2017

@JedWatson is there any schedule/plan to merge/fix this issue?

inoas commented Jun 29, 2017

@JedWatson is there any schedule/plan to merge/fix this issue?

121watts added a commit to influxdata/chronograf that referenced this issue Jul 14, 2017

WIP Edit scripts returned from the server
Had to downgrade react-codemirror due to a bug.  Issues and PRs
are open to resolve:
- JedWatson/react-codemirror#121
- JedWatson/react-codemirror#106
@hankthewhale

This comment has been minimized.

Show comment
Hide comment
@hankthewhale

hankthewhale Jul 19, 2017

@MartinHaeusler We ran into that issue too with the v1 update, introduced by this commit 339b597. Your best bet is to either...

  1. use ^0.3.0 and suffer the deprecation warnings
  2. roll your own codemirror wrapper (that's what we ended up doing)

Bummer that we had abandon this project but supporting your own wrapper is actually not too difficult.

hankthewhale commented Jul 19, 2017

@MartinHaeusler We ran into that issue too with the v1 update, introduced by this commit 339b597. Your best bet is to either...

  1. use ^0.3.0 and suffer the deprecation warnings
  2. roll your own codemirror wrapper (that's what we ended up doing)

Bummer that we had abandon this project but supporting your own wrapper is actually not too difficult.

@MartinHaeusler

This comment has been minimized.

Show comment
Hide comment
@MartinHaeusler

MartinHaeusler Jul 19, 2017

@hankthewhale thanks for the info. I downgraded the package a while ago. Overall I think that code-mirror is great, but to use it properly with react, we would need a full "controlled component" version that has no internal state and receives everything via props and callbacks. Unfortunately this wrapper here does not offer a lot of these features, and code-mirror in particular is quite a big component.

MartinHaeusler commented Jul 19, 2017

@hankthewhale thanks for the info. I downgraded the package a while ago. Overall I think that code-mirror is great, but to use it properly with react, we would need a full "controlled component" version that has no internal state and receives everything via props and callbacks. Unfortunately this wrapper here does not offer a lot of these features, and code-mirror in particular is quite a big component.

@scniro

This comment has been minimized.

Show comment
Hide comment
@scniro

scniro Jul 19, 2017

@MartinHaeusler like I had mentioned earlier in this thread - react-codemirror2 is exactly as you describe - all through props and callbacks. I've recently updated the docs and demo site - give it a look.

scniro commented Jul 19, 2017

@MartinHaeusler like I had mentioned earlier in this thread - react-codemirror2 is exactly as you describe - all through props and callbacks. I've recently updated the docs and demo site - give it a look.

@hankthewhale

This comment has been minimized.

Show comment
Hide comment
@hankthewhale

hankthewhale Jul 20, 2017

@marcioaffonso I think this lib only manages a focus state locally. Outside of the current bug for prop value comparison, it is fully controlled.

But yeah, other than downgrading or rolling your own, check out @scniro's lib.

hankthewhale commented Jul 20, 2017

@marcioaffonso I think this lib only manages a focus state locally. Outside of the current bug for prop value comparison, it is fully controlled.

But yeah, other than downgrading or rolling your own, check out @scniro's lib.

Dentosal added a commit to Dentosal/react-codemirror that referenced this issue Jul 20, 2017

Merge pull request #1 from skidding/106-fix-update
No longer debounce componentWillReceiveProps JedWatson#106
@dandersonstack

This comment has been minimized.

Show comment
Hide comment
@dandersonstack

dandersonstack Jul 21, 2017

@skidding you version of codemirror worked perfectly! yay. we were having so many issues with this.
We were able to roll back to 0.2.6, and it worked, but then when we moved our state into a redux store, and codemirror stopped updating :(.

Fortunately when using import CodeMirror from '@skidding/react-codemirror';
Everything worked perfectly!!
For those curious...

In the end it was simple as this:

      <CodeMirror 
        value={this.props.code}
        onChange={this.codeChange}
        options={options} />
    );

, but it only worked using that library I referenced earlier. Hopefully that pull request will fix the issue, please update here when it has been merged.

dandersonstack commented Jul 21, 2017

@skidding you version of codemirror worked perfectly! yay. we were having so many issues with this.
We were able to roll back to 0.2.6, and it worked, but then when we moved our state into a redux store, and codemirror stopped updating :(.

Fortunately when using import CodeMirror from '@skidding/react-codemirror';
Everything worked perfectly!!
For those curious...

In the end it was simple as this:

      <CodeMirror 
        value={this.props.code}
        onChange={this.codeChange}
        options={options} />
    );

, but it only worked using that library I referenced earlier. Hopefully that pull request will fix the issue, please update here when it has been merged.

@inoas

This comment has been minimized.

Show comment
Hide comment
@inoas

inoas Jul 24, 2017

Edit: Please try https://github.com/scniro/react-codemirror2 instead.
That fork is maintained, available via npm and gathering stars.

I can confirm that @skidding/react-codemirror works. Newbies guide:

  • In your app code replace import CodeMirror from 'react-codemirror'; with import CodeMirror from '@skidding/react-codemirror';
  • delete package-lock.json (there are probably better ways)
  • in package.json replace "react-codemirror": "^1.0.0", with "@skidding/react-codemirror": "^1.0.0",
  • run npm install, run webpack/wait for compile/reload bowser
  • redux store values are correctly swapping (for us on locale switching)

inoas commented Jul 24, 2017

Edit: Please try https://github.com/scniro/react-codemirror2 instead.
That fork is maintained, available via npm and gathering stars.

I can confirm that @skidding/react-codemirror works. Newbies guide:

  • In your app code replace import CodeMirror from 'react-codemirror'; with import CodeMirror from '@skidding/react-codemirror';
  • delete package-lock.json (there are probably better ways)
  • in package.json replace "react-codemirror": "^1.0.0", with "@skidding/react-codemirror": "^1.0.0",
  • run npm install, run webpack/wait for compile/reload bowser
  • redux store values are correctly swapping (for us on locale switching)

121watts added a commit to influxdata/chronograf that referenced this issue Jul 25, 2017

WIP Edit scripts returned from the server
Had to downgrade react-codemirror due to a bug.  Issues and PRs
are open to resolve:
- JedWatson/react-codemirror#121
- JedWatson/react-codemirror#106
@laduke

This comment has been minimized.

Show comment
Hide comment
@laduke

laduke Jul 28, 2017

Another work around: add a key prop to the codemirror component. Then key = key + 1 when the value prop changes. This makes the editor re-mount. It's ok for me because the value prop only changes when the user clicks the 'reset' button.

laduke commented Jul 28, 2017

Another work around: add a key prop to the codemirror component. Then key = key + 1 when the value prop changes. This makes the editor re-mount. It's ok for me because the value prop only changes when the user clicks the 'reset' button.

121watts added a commit to influxdata/chronograf that referenced this issue Aug 7, 2017

WIP Edit scripts returned from the server
Had to downgrade react-codemirror due to a bug.  Issues and PRs
are open to resolve:
- JedWatson/react-codemirror#121
- JedWatson/react-codemirror#106

cpunion added a commit to cpunion/react-codemirror that referenced this issue Aug 9, 2017

121watts added a commit to influxdata/chronograf that referenced this issue Sep 7, 2017

WIP Edit scripts returned from the server
Had to downgrade react-codemirror due to a bug.  Issues and PRs
are open to resolve:
- JedWatson/react-codemirror#121
- JedWatson/react-codemirror#106

121watts added a commit to influxdata/chronograf that referenced this issue Sep 12, 2017

WIP Edit scripts returned from the server
Had to downgrade react-codemirror due to a bug.  Issues and PRs
are open to resolve:
- JedWatson/react-codemirror#121
- JedWatson/react-codemirror#106
@inoas

This comment has been minimized.

Show comment
Hide comment
@inoas

inoas Oct 2, 2017

Edit: Please try https://github.com/scniro/react-codemirror2 instead.
That fork is maintained, available via npm and gathering stars.

inoas commented Oct 2, 2017

Edit: Please try https://github.com/scniro/react-codemirror2 instead.
That fork is maintained, available via npm and gathering stars.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment