Page MenuHomeElementl

Fix undo functionatlity in Dagit playground
AbandonedPublic

Authored by sashank on Nov 25 2020, 9:33 PM.

Details

Summary

Resolves https://github.com/dagster-io/dagster/issues/3294. This diff patches react-codemirror2 to get rid it's weird behavior that resets the user history when the document is updated

Test Plan

Select partition, verify that undo works now

Diff Detail

Repository
R1 dagster
Branch
cm-undo
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

This patch package is pretty intriguing. I'm OK with doing this in this isolated instance because we're likely to abandon codemirror at some point in the future (for Monaco) and forking / fixing the library to make this change seems like overkill. I'm curious to get @dish 's thoughts + approval on this though because he investigated that change.

We should also double check that switching tabs in the editor doesn't create a continuous undo history across documents now. (Previously, I believe that switching tabs ALSO destroyed your undo history, so if it still does, that would be OK)

Double check that switching tabs in the editor doesn't create a continuous undo history

Good call! Will verify this

I'm a little confused about why a controlled react-codemirror doesn't allow simply setting the value. Isn't that what the controlled component *should* want you to do?

Oh wow, yeah, I see it. That's really annoying.

An idea, based on https://github.com/scniro/react-codemirror2/issues/175: Use this._editor in ConfigEditor to manually call this._editor.replaceRange() and set the new value. I'm not totally sure how you'd know when to call replaceRange for a given value change. It might have to be something like a public method on ConfigEditor, though it looks like there's already some precedent for that.

How would you feel about just copying over react-codemirror2 into a vendor directory, instead of patching the package this way? Then we can do whatever we need to, without trying to patch, while we plot a move to Monaco?

Sounds good, will fork the package instead