JedWatson / react-codemirror

Codemirror Component for React.js
MIT License
1.55k stars 262 forks source link

Why is Codemirror#componentWillReceiveProps async? #47

Open n1k0 opened 8 years ago

n1k0 commented 8 years ago

There might be a very good reason I'm not aware of for debouncing componentWillReceiveProps by default, but it created a lot of headache when it comes to state synchronization (refs https://github.com/mozilla-services/react-jsonschema-form/pull/175).

What's the reasoning behind scheduling it to the next tick, perfs? Could it be conceivable to make this an optional behavior?

sslotsky commented 8 years ago

The debouncing causes me some trouble as well. It appears that the debounce applies to all instances of the component on a page simultaneously, so if they all try to re-render at once, only the last instance will actually show a change.

This was the issue I described in #46, which I closed because I found a way to work around it. However, this will still be a problem for instances like drag & drop sorting, where there is more than one instance that legitimately needs to re-render at the same time.

Here's a link to a video demonstrating the issue.

https://youtu.be/-mbHy17bxZw

sslotsky commented 8 years ago

I submitted #49 to make the debounce specific to the instance, rather than all instances.

This resolves the issue shown in the video linked above, as can be seen in this video.

https://drive.google.com/open?id=0Bzbw-6Q_sVTyblQzMldidXdIdUU

attaboy commented 8 years ago

Thanks for the pull request, @sslotsky. It fixes a similar problem I was having where a first instance of CodeMirror fails to sync its options to state when there's a second instance on the page.

alexdmiller commented 8 years ago

I submitted the PR which added the debouncing—it could have been misguided. I was having a nightmare with state synchronization, as described in #5. I wrote #35 to fix my problems. I didn't consider what would happen with multiple instances.

Maybe someone else can dig into the state sync problems and come up with a better solution.

Chun-Yang commented 7 years ago

Hey @alexdmiller what was the reason to debounce?

If there is a good reason, one way to fix the bug is to create the debounce function in the componentDidMount function, I can give it a shot. But before that, could you elaborate the reason for the debounce?

Aaronius commented 7 years ago

This seems really tricky to work around. For example, in my component I have a componentDidUpdate where I'm calling doc.markText(), but this assumes that the underlying CodeMirror instance has the value that was passed into the React-CodeMirror instance during the last render. This is problematic though, because React-CodeMirror doesn't pass the value to the underlying CodeMirror instance until its debounce is complete, which is after my componentDidUpdate execution.

scniro commented 7 years ago

This appears to be a duplicate of #106. I ran into the same issue and created react-codemirror2 because I was dying over the debouncing issue (and couldn't sleep at night with the lodash dependency). My wrapper component is very barebones, surely needs some more work, but works well for my simple usage. I'll be maintaining it moving forward so any struggles going on here I'll happily work out in the alternative package. Please let me know if anyone takes the time to check it out and finds it useful - thanks.