Unify build api code #572
Merged
Unify build api code #572
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
schanzer
reviewed
Nov 11, 2021
| ): Result<{ oldAst: AST; newAst: AST; newCode: string }, string> { | ||
| let oldAst: AST; | ||
| try { | ||
| oldAst = AST.from(language.parse(oldCode)); // parse the code (WITH annotations) |
There was a problem hiding this comment.
This is where the old annotate argument came in handy -- we just parsed without building any annotations, to make sure it was valid code. IIRC, the annotation process was pretty expensive. I saw noticeable jank when dealing with programs >400LOC.
schanzer
reviewed
Nov 11, 2021
| @@ -135,36 +132,34 @@ const BlockEditor = ({ options = {}, ...props }: BlockEditorProps) => { | |||
| * When the editor mounts, (1) set change event handlers and AST, | |||
| * (2) set the focus, (3) set aria attributes, and (4) build the API | |||
| */ | |||
| const handleEditorDidMount = (editor: CodeMirrorFacade) => { | |||
| const handleEditorDidMount = (codemirror: CodeMirror.Editor) => { | |||
| const editor = new CodeMirrorFacade(codemirror); | |||
| setEditor(editor); | |||
| // TODO(Emmanuel): Try to set them in the component constructor | |||
There was a problem hiding this comment.
This is the only place where we manually use CM's methods to attach handlers. It's ugly. I wasn't able to convince TypeScript that setting these in the constructor was a valid move. I suspect you'll have a better shot at figuring it out. :)
|
Ok, I think this is ready to merge. Want to take a look at it @schanzer? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves all the "api building" code to one place, CodeMirror-api.tsx, and moves the call site(s) for this code to the top of the component hierachry.
Previously, the imperative API that we exposed as a replacement for and augmentation of codemirror's apis, was tightly coupled to various react components and their internal state and event handlers. To get all the different bits and pieces together required a lot of plumbing between components to pass the relevant parts of the public API up through the hiearchy. Now the APIs are just a wrapper around the redux store and a codemirror editor... they do not have anything to do with react. Likewise, the react components now don't have to care or know anything about the public interface to the library.
There's still some more cleanup that I'd like to do before merging this, so leaving it in draft mode for now.