Experimental CodeMirror editor#1670
Conversation
|
@wylieconlon relevant to your interests? mind taking a look? |
|
Yes, I'll take a look as soon as I can. Thanks for putting this out!
…On Thu, Mar 14, 2019 at 3:28 PM Mat Brown ***@***.***> wrote:
@wylieconlon <https://github.com/wylieconlon> relevant to your interests?
mind taking a look?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1670 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAora5HaiVwpjIOJLtm_Z644igO647vuks5vWqLsgaJpZM4b1Cxd>
.
|
wylieconlon
left a comment
wylieconlon
left a comment
There was a problem hiding this comment.
Overall I like the direction of this, my biggest concern is whether we can easily test the various effects without relying too heavily on Ace or CodeMirror APIs. My secondary concern is that a class component might have easier-to-follow logic, for example:
componentDidMount() {
this._editor = CodeMirror(this.editorContainer.current, {
mode: 'javascript',
theme: 'monokai',
value: this.props.source,
lineNumbers: true,
lineWrapping: true,
});
That being said, it's definitely the smaller concern. I'd just like to see your thoughts on both testing and class components.
gulpfile.js
Outdated
| const distDir = 'dist'; | ||
| const stylesheetsDir = path.join(srcDir, 'css'); | ||
| const highlightStylesheetsDir = 'node_modules/highlight.js/styles'; | ||
| const codemirrorStylesheet = 'node_modules/codemirror/lib/codemirror.css'; |
There was a problem hiding this comment.
There are multiple built-in color schemes for codemirror that we could use as well- the default is pretty low contrast. For light schemes, I prefer Eclipse or Idea, but you can take a look at the built ins: https://codemirror.net/demo/theme.html
There was a problem hiding this comment.
I agree that the default scheme isn't amazing, but I actually went through all the themes that ship with the package and didn't find any I thought were overall better than the default (I also liked the IDE-derived themes but there were things I didn't like that I don't remember now; I think they were mostly in the way they themed HTML). This repository of themes looks promising though...
Anyway, seems like something we can continue to ponder between initial experimental-mode release and actually making this the official editor.
There was a problem hiding this comment.
The default scheme is specifically less contrasty than these other options, which made it noticeable when switching back and forth. Totally fine with me to update later
.eslintrc
Outdated
| "plugins": [ | ||
| "import", | ||
| "react", | ||
| "react-hooks", |
There was a problem hiding this comment.
If you want to start supporting hooks, should that be merged in separately?
There was a problem hiding this comment.
So my basic thinking is:
- Hooks are good and class components are bad (more on that below)
- We should therefore not introduce any new class components, and when we need the features that require either hooks or class components in a new component, we should use hooks
- And soon we should probably transition fully to hooks even though the React folks say not to go all crazy rewriting components for hooks just yet
- So, with that in mind, this is the first time since hooks became a thing that we have a new component that needs to use hooks. So, this would be the exact time to "start supporting hooks" (I don't see much argument for, say, introducing the eslint plugin when there are no actual hooks to lint)
There was a problem hiding this comment.
Since writing this I'm now developing almost entirely in hooks, I think I'll write up some thoughts on that on a higher-up comment.
gulpfile.js
Outdated
| path.join(highlightStylesheetsDir, 'github.css'), | ||
| codemirrorStylesheet, | ||
| path.join(stylesheetsDir, '**/*.css'), | ||
| ]). |
There was a problem hiding this comment.
Because there are multiple codemirror stylesheets, I think we want to define them all using something like this:
src(bowerStylesheets.concat(highlightStyleSheets, codemirrorStylesheets, sourceStylesheets))
There was a problem hiding this comment.
Are you cool with what is there now? I'm not sure what the key difference is between what I had before and what you wrote in the above comment.
There was a problem hiding this comment.
Based on our discussion of adding stylesheets later this is fine
package.json
Outdated
| "loadjs": "^3.5.4", | ||
| "lodash-es": "^4.17.11", | ||
| "loop-breaker": "^0.1.0", | ||
| "lru-cache": "^5.1.1", |
There was a problem hiding this comment.
Used to cache CodeMirror.Doc instances. Basically I want to hold on to the last few, so that the undo buffer is retained if you're switching back and forth between a couple projects, but don't want to build up an indefinitely growing collection of them.
| import 'brace/mode/html'; | ||
| import 'brace/mode/css'; | ||
| import 'brace/mode/javascript'; | ||
| import 'brace/theme/monokai'; |
There was a problem hiding this comment.
Where are these styles?
There was a problem hiding this comment.
I don't actually know, and I'm not sure if this line of code even does anything. In CodeMirror, monokai is a dark theme...
src/components/CodeMirrorEditor.jsx
Outdated
| @@ -0,0 +1,90 @@ | |||
| import CodeMirror from 'codemirror'; | |||
| import isNil from 'lodash-es/isNil'; | |||
src/components/CodeMirrorEditor.jsx
Outdated
| const mode = CODEMIRROR_MODES_MAP[language]; | ||
|
|
||
| useEffect(() => { | ||
| const {current: container} = containerRef; |
There was a problem hiding this comment.
I would normally avoid stylistic comments, but this one is bothering me. Why destructure when the key isn't used? I prefer const container = containerRef.current; and think it would simplify all the effects code
There was a problem hiding this comment.
Quite right! I ended up rewriting most of these already, but will seek out any lingerers. I also added an entry to the contributor guide proposing some guidelines for when destructuring is useful.
src/components/CodeMirrorEditor.jsx
Outdated
| container, | ||
| { | ||
| lineNumbers: true, | ||
| lineWrapping: true, |
There was a problem hiding this comment.
You haven't provided a theme name or mode to the initialize call- I think you can, since the props are available here
There was a problem hiding this comment.
As for a theme name, as I mentioned, I have yet to land on a theme that is obviously preferable to the default theme.
For the mode, it would feel good to put it here, but that would make the language prop an effect dependency, which we definitely don't want here! Though practically we would never expect language to change, if it did in fact change, we should not tear down and re-create the editor instance; we should just swap in a new Doc instance, with the appropriate mode. And that's exactly what the following effect hook does.
There was a problem hiding this comment.
That wasn't obvious to me on first review, I'd appreciate a comment on why the mode is missing
src/components/CodeMirrorEditor.jsx
Outdated
| const {current: container} = containerRef; | ||
|
|
||
| const editor = editorRef.current = CodeMirror( | ||
| container, |
There was a problem hiding this comment.
In this case, I don't think you need to unwrap the reference at all. containerRef.current would work fine here
There was a problem hiding this comment.
Something about refs makes me always want to unwrap them, basically so I don't have to look at the ref itself any more than necessary. Also, in some cases it actually does matter (e.g. in effect cleanup functions) so I think it's simpler to just have a global rule of "always unwrap ref values into their own variable" and then you don't have to worry about it.
src/components/CodeMirrorEditor.jsx
Outdated
| if (doc !== editor.getDoc()) { | ||
| editor.swapDoc(new CodeMirror.Doc('', mode)); | ||
| } | ||
| }, [language, mode, projectKey]); |
|
@wylieconlon thanks for the thoughtful and detailed code review! i actually had a long flight yesterday and I think I got the CM editor close to parity with functionality with ACE, but didn't get a chance to push it until this morning. But I will definitely go through your review, respond to stuff, and make changes as appropriate : ) |
So after reading Dan Abramov's Big Blog Post I am pretty strongly convinced that class components are something approaching an antipattern (specifically the notion of Anyway, I believe this concludes all comment responses I owe you, looking forward to hearing your thoughts! |
wylieconlon
left a comment
wylieconlon
left a comment
There was a problem hiding this comment.
Overall this is very clean, and I don't have any major concerns with this. I've been using hooks much more at work, so it was easier to read this code the second time around. I'm in favor of moving more components towards hook-based logic.
|
|
||
| /* postcss-bem-linter: ignore */ | ||
| .CodeMirror-activeline-background { | ||
| background: var(--color-low-contrast-gray) !important; |
There was a problem hiding this comment.
I hadn't realized we were using css variables, cool!
| import ShallowRenderer from 'react-test-renderer/shallow'; | ||
| import TestRenderer, {act} from 'react-test-renderer'; | ||
|
|
||
| // eslint-disable-next-line import/no-extraneous-dependencies |
There was a problem hiding this comment.
There’s a fix in the master branch of eslint-plugin-import that has yet to be released; once that lands this disable will no longer be necessary (and should itself trigger a lint error for no unnecessary disables)
jwang1919
left a comment
jwang1919
left a comment
There was a problem hiding this comment.
Couldn't comment on the CodeMirror part because I don't know about that library either, just looked into things that could be improved.
|
|
||
| _scrollToLine(lineNumber) { | ||
| const shouldCenterVertically = true; | ||
| const shouldAnimate = true; |
There was a problem hiding this comment.
If these two consts will always be true, why not just directly set both to true as arguments? If it's supposed to be configurable in the future, these values should be located in a properties/config file.
There was a problem hiding this comment.
Not sure/good point but this is basically just moving the old Editor component into a new place, and we’re deprecating it anyway, so I don’t think there’s any need to worry about this question
|
|
||
| /* postcss-bem-linter: ignore */ | ||
| .CodeMirror-activeline-background { | ||
| background: var(--color-low-contrast-gray) !important; |
There was a problem hiding this comment.
Can you avoid using !important? Also applies to line 565,
There was a problem hiding this comment.
I don’t think so—generally using !important is bad but in this case we are overriding a style in a vendor stylesheet so I don’t think there is any alternative.
outoftime
force-pushed
the
codemirror
branch
2 times, most recently
from
60a6c95 to
42ad577
Compare
outoftime
force-pushed
the
codemirror
branch
2 times, most recently
from
6c15224 to
4db91c5
Compare
|
N.B. I did a quick apples-to-apples on this and it looks like the bundle size will be about 10% smaller with CodeMirror fully replacing Ace. |
outoftime
force-pushed
the
codemirror
branch
8 times, most recently
from
7e34780 to
030560c
Compare
Creates a basic CodeMirror-backed editor component. Currently hard-coded to replace the old editor component but should actually be lazy-loaded only in experimental mode. Lacks most of the features of the old editor component so far, but bare-bones functionality is there.
Uses the `lint` addon, which basically does the right thing. Interface is a little strange in that you have to give it a function that returns a collection of errors (you cannot set errors imperatively). So in effect-world this means updating the editor configuration each time errors change, providing a new function to return the new set of errors. This seems to work well enough.
outoftime
force-pushed
the
codemirror
branch
from
030560c to
a2770ce
Compare
3 participants
Creates an experimental
<CodeMirrorEditor>component, which accepts the same (actually currently a subset of) the props as<Editor>but uses CodeMirror rather than ACE as the underlying editor implementation.This is an initial spike and is not intended to be full-featured or release-ready. The specific goals of this pull request are:
We do get several other things for free/by default with CodeMirror. The full list of features that are required for parity with the ACE implementation can be found in the checklist in #1434.
In this pull request (and until that checklist is complete), the CodeMirror editor is only used in experimental mode (when the
experimentalquery param is present in the URL). In order to avoid bloating the bundle unnecessarily, the<CodeMirrorEditor>component is loaded asynchronously and lazily, in its own chunk, only in experimental mode. This means that the interface takes slightly longer to fully load in experimental mode.