Improve editor load and keep Monaco features stable.#1658
Conversation
Load important Monaco parts first, move heavy parts to background loading, and fix local dev service worker behavior so editor features stay reliable.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1658 +/- ##
==========================================
- Coverage 95.38% 95.29% -0.10%
==========================================
Files 34 36 +2
Lines 7839 7943 +104
==========================================
+ Hits 7477 7569 +92
- Misses 362 374 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@elalish Changed Monaco loading to lazy loading Before: big Monaco parts were loaded early. Editor API, TypeScript contribution, and workers now load through loadMonacoModules(). Added ensureMonacoSuggestLoaded(). Added ensureMonacoNavigationLoaded(). editor.all is still used, but now loaded later with ensureMonacoContributionsLoaded(). monaco-editor-auto-typings is now imported lazily in initializeAutoTypings(). startEnhancements() runs once and starts: Added skipServiceWorker logic for: Added Monaco CSS imports for editor and tokens. Replaced wrong module/moduleResolution setup with: Added guard in getModelForScript() if Monaco is not ready yet. In short: this PR makes initial load lighter, keeps suggestions/navigation reliable, fixes local dev service worker issues, and restores proper editor colors. |
|
Thanks, before I review this in more detail, would you mind showing some before/after of the chrome dev tools performance metrics and perhaps the page load graphics? Also, can you describe as a user the difference you see, particularly if you throttle your network? How soon do the buttons react? When is the script visible? How long until it runs and shows the model? |
the videos are on 5.00x speed: before: https://drive.google.com/file/d/1PSJj8bFbghHWk_0pfXyWuF0xop7ED-Gc/view?usp=drive_link after: https://drive.google.com/file/d/11VwLIDYdlaXV_7mxZzJbYE5oo4eksY-U/view?usp=drive_link
|
| const {monaco: monacoApi, editorWorker, tsWorker} = await loadMonacoModules(); | ||
| monaco = monacoApi; | ||
| await ensureMonacoSuggestLoaded(); | ||
| await ensureMonacoNavigationLoaded(); |
There was a problem hiding this comment.
In your after video, it appears that esbuild.wasm is only triggered to download after the Run button is pressed. Can that also be proactively pulled down in the background with everything else?
There was a problem hiding this comment.
In your after video, it appears that esbuild.wasm is only triggered to download after the Run button is pressed. Can that also be proactively pulled down in the background with everything else?
yes , i am going to start work on this ,and i will make a video
|
video is on 2.00x speed: after update : https://drive.google.com/file/d/1Xz7yoWi_84SY78WJPn_Rcm8kyNEKEKNJ/view?usp=drive_link |
elalish
left a comment
There was a problem hiding this comment.
This is looking pretty ready to me, except for the syntax highlighting issue. Can you do a before/after pass and look for anything else that may have changed in the UX?
| enhancementsStarted = true; | ||
| // Start downloads in background; don't block editor interactivity. | ||
| ensureEsbuildWasmPreloaded().catch(error => { | ||
| console.warn('Failed to preload esbuild.wasm:', error); |
There was a problem hiding this comment.
Not for this PR, but esbuild.wasm looks like it's pulling 13MB or so, which is ridiculous. It would be interesting to see if there's some way we could pare that down.
There was a problem hiding this comment.
Yeah... Unfortunately that pretty much means replacing it.
| async function createEditor() { | ||
| const {monaco: monacoApi, editorWorker, tsWorker} = await loadMonacoModules(); | ||
| monaco = monacoApi; | ||
| await ensureMonacoSuggestLoaded(); |
There was a problem hiding this comment.
Not for this PR, but I believe there are multiple ways to load Monaco. I believe we're now loading more of their code than we're actually using, so it would be great to look into minimizing that. Might be good to make a little chart of our download sizes and what they're used for.
There was a problem hiding this comment.
Not for this PR, but I believe there are multiple ways to load Monaco. I believe we're now loading more of their code than we're actually using, so it would be great to look into minimizing that. Might be good to make a little chart of our download sizes and what they're used for.
yes , we will work on this
elalish
left a comment
There was a problem hiding this comment.
Looks good, just a couple of minor nits.
|
|
||
| async function createEditor() { | ||
| const {monaco: monacoApi, editorWorker, tsWorker} = await loadMonacoModules(); | ||
| monaco = monacoApi; |
There was a problem hiding this comment.
I think you're just renaming this and then back again, right?
There was a problem hiding this comment.
Yes Sir
I destructured as monacoApi to avoid confusion with the outer monaco variable, then assigned to the module-level monaco,I will simplify naming.
| minimap: {enabled: false}, | ||
| quickSuggestions: true, | ||
| suggestOnTriggerCharacters: true, | ||
| parameterHints: {enabled: true}, |
There was a problem hiding this comment.
These look interesting - how did you decide which to use?
There was a problem hiding this comment.
These were chosen to restore expected Monaco TS UX while keeping imports minimal:
quickSuggestions: shows completions while typing (no manual trigger needed).
suggestOnTriggerCharacters: supports . / import-path trigger behavior.
parameterHints: function signature help while typing arguments.
I enabled only these because they are low-cost and directly tied to the delayed-intellisense issue.
| let monacoContributionsReady = false; | ||
| let autoTypings = undefined; | ||
| let autoTypingsPromise = undefined; | ||
| let esbuildWasmPreloadPromise = undefined; |
There was a problem hiding this comment.
I prefer to use null instead of undefined pretty much across the board. It tends to be easier to debug and even runs a touch faster, plus it's easier to type.
There was a problem hiding this comment.
ok , i am going to replace it with null.
tonious
left a comment
There was a problem hiding this comment.
Looks good! There's a few small comments in here, but they're all optional.
| enhancementsStarted = true; | ||
| // Start downloads in background; don't block editor interactivity. | ||
| ensureEsbuildWasmPreloaded().catch(error => { | ||
| console.warn('Failed to preload esbuild.wasm:', error); |
There was a problem hiding this comment.
Yeah... Unfortunately that pretty much means replacing it.
| let updateTypeIndicator = () => {}; | ||
|
|
||
| async function loadMonacoModules() { | ||
| if (!monacoModulesPromise) { |
There was a problem hiding this comment.
These all follow the same kind of pattern, which is basically a memoize...
It's possible to extract that out. Something like:
async function memoizeAsync(fn) {
let promise = null;
return async () => {
if (!promise) promise = fn();
return promise;
}
}
async function loadMonacoModules() {
return Promise.all([
// .... Stuff goes here.
]);
}
const ensureLoadMonacoModules = memoizeAsync(loadMonacoModules);Typed, but not tested.
If this approach makes the PR more readable to you, great! If not, that's fine too -- this is just another technique that may help someday.
| const isLocalhost = window.location.hostname === 'localhost' || | ||
| window.location.hostname === '127.0.0.1'; | ||
| const skipServiceWorker = | ||
| disableServiceWorker || import.meta.env.DEV || isLocalhost; |
There was a problem hiding this comment.
I understand using the term skipServiceWorker as it encompasses more than just disableServiceWorker. But I think I'd prefer to call out the cases individually.
const disableServiceWorker = params.has('no-sw');
const isLocalhost = window.location.hostname === 'localhost' ||
window.location.hostname === '127.0.0.1';
const isDevEnv = import.meta.env.DEV;This makes the check more explicit:
// Disable the service worker if asked, in development or
// on localhost (in which case you can work offline anyhow).
if (disableServiceWorker || isDevEnv || isLocalhost) {|
@tonious I changed the service worker check to be more clear (disableServiceWorker, isDevEnv, isLocalhost) and used them directly in the if condition. Thanks again, your suggestion helped make this part cleaner. |
|
@elalish |


Load important Monaco parts first, move heavy parts to background loading, and fix local dev service worker behavior so editor features stay reliable.