Rename advanced editor to Ace and add Monaco #736
Conversation
|
@shepmaster Do you disagree with this? Should I close this PR? |
| @@ -36,7 +36,8 @@ export interface CommonEditorProps { | |||
|
|
|||
| export enum Editor { | |||
| Simple = 'simple', | |||
| Advanced = 'advanced', | |||
There was a problem hiding this comment.
Doing it this way will require changes to local_storage.ts to preserve people's current selections. If you don't feel like making those changes, you could also leave the value as advanced and solely add monaco.
There was a problem hiding this comment.
I added a migration to local_storage.ts.
ui/frontend/editor/MonacoEditor.tsx
Outdated
| const modeId = 'my-rust'; | ||
|
|
||
| const initMonaco = (monaco: Monaco) => { | ||
| monaco.editor.defineTheme('vscode-dark-plus', { |
There was a problem hiding this comment.
Why do we need to create our own theme? What isn't provided by upstream?
There was a problem hiding this comment.
VSCode themes are not compatible with monaco because they use textmate convention for tokens but monaco has its own (funnily, monaco is directly built from vscode source code) so upstream won't provide vscode themes. Original themes are ugly (IMO) and people are more familiar with vscode theme.
That theme is not exactly equal to vscode's dark-plus, but it is close enough and I think it is good for default theme (and user can change theme). But we can remove those nine lines if there is something wrong with them.
| @@ -0,0 +1,142 @@ | |||
| export const config = { | |||
There was a problem hiding this comment.
Does this mean that there's no upstream Rust configuration for Monaco?
There was a problem hiding this comment.
There is, but I'm not happy with it. Some can be solved in upstream (for example not auto closing ' in lifetimes, or more detailed grammar) and some of changes are related to difference of textmate and monarch tokens so won't be accepted in upstream.
Nope and nope! Life is busy, is all. |
|
Starting from the main branch then checking out this code yields numerous console errors: |
|
This appears to load JS resources from a CDN:
Please adjust the imports so that the JS is served by us. |
|
There appear to be some commands that won't do anything:
Please remove these (and any other inoperable actions) to avoid bugs being filed that they "don't work". |
|
Ignore this now-deleted comment — I got confused that the filename contained |
It seems that special commands can't be removed. Whole command palette can be removed, but most of command in it works. Even those commands you mentioned work technically, as they work in vscode. Vscode still shows this command when there is no problem and it does nothing when we click (exactly like here). When we add RA, some problems are added so it comes out of the "always does nothing" mode (still if I find a way to remove commands, I'll remove these commands). So I think keeping command palette is better than removing it completely? CDN and migration should be solved now. |
|
The Monaco JS and CSS are now served by us, but they aren't being properly fingerprinted:
Contrast this with the Ace files:
We apply aggressive caching headers ( |
|
I added a hash to the folder name. It is suboptimal but |
|
Why the choice of |
ui/frontend/editor/AceEditor.tsx
Outdated
| @@ -448,7 +448,7 @@ class AdvancedEditorAsync extends React.Component<AdvancedEditorAsyncProps, Adva | |||
| private async requireLibraries() { | |||
| return import( | |||
| /* webpackChunkName: "advanced-editor" */ | |||
| './advanced-editor' | |||
| '../advanced-editor' | |||
There was a problem hiding this comment.
these should be renamed to remain consistent.
There was a problem hiding this comment.
Probably grep for "advanced" — there's AdvancedEditorAsyncState down below as well.
|
There was something wrong with monaco webpack plugin, IIRC lazy loading was hard (which |
|
Looking at the current lazy-loading, it's super old, from before React's |
|
I got something working in https://github.com/integer32llc/rust-playground/tree/ace-monaco:
It's a bit bigger than yours, however (1.1 MB vs 815 KB). We do have more control over what features are present, so maybe there's something we don't need? |
Ah, including Monaco twice will do that. Now it's at 675/713 KB |
|
I was misguided that it won't work with normal webpack dynamic import. Now it is based on your branch.
I'm still getting bigger bundles. What do you mean by "including twice"? |
There are two versions of monaco-editor. The versions need to be unified. |
|
I've force-pushed to your branch after moving some of the commits around and doing general cleanup. The files are now fingerprinted or inlined:
I also moved all the Ace settings into The next step will be adding some automated UI tests. |
|
Great!
The ruby ones? I don't know ruby, but I can copy existing ones. Also I'm not sure what need to be tested / can be tested. |
Yes, those. Getting good tests is hard and I'm happy to write those.
This is the more important part I'll need from you — we don't need to deeply test Monaco itself, but what kinds of things would you do to ensure that it was correctly integrated? |
|
hmm, I think if it types something, run fmt, type some more, change the theme, run the code, then if result and code are as expected, and some color changes, and console is clean, then everything our side should be ok. Aside monaco, migrations and preserving config in editor switches need test. |
|
@shepmaster ping |
|
@HKalbasi thanks for the ping. I've rebased and added some small tweaks. There's a unit test for the migration and an integration test using Monaco. Care to pull it locally and see if everything continues to work as you expect? |
shepmaster
added
CI: approved
|
Everything looks good |
shepmaster
added
CI: approved
shepmaster
added
CI: approved
shepmaster
added
CI: approved
|
Let’s go! |
|
For anyone who wants to give it a try, this is already live on https://play.rust-lang.org/. |
|
Generally things should always be live <1hour after merging. You may need to aggressively reload the page to break the caching, otherwise waiting an hour or day should work. |
|
Is there a way to set the editor via GET param, e. g. |
|
I don't think it should be. All configuration options are personal preferences and shouldn't be overridable by playground links if you don't pay attention to the full url. |
This is first phase of #357
It should have zero impact to users (except 2 or 3 kilobytes more download). If people want to use it they should manually opt-in.
Closes #450