Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix downloading monaco twice per load #267

Merged
merged 5 commits into from
Feb 3, 2025
Merged

Fix downloading monaco twice per load #267

merged 5 commits into from
Feb 3, 2025

Conversation

WardBrian
Copy link
Collaborator

I noticed some slow loading during a dev environment that made me inspect the network tab and realize monaco was not loading from localhost, but from a CDN.

After some digging, it turns out @monaco-editor/react uses a package called @monaco-editor/loader that has this behavior. I was unsuccessful in getting it to use our bundled monaco, so instead I went the other way and made it so we don't have to bundle monaco-editor ourselves at all.

The bundle size is much happier now, here's yarn build:

dist/assets/__vite-browser-external-9wXp6ZBx.js      0.03 kB
dist/index.html                                      0.69 kB │ gzip:   0.37 kB
dist/assets/StanModelWorker-DlhrIcAU.js              6.88 kB
dist/assets/pyodideWorker-C-8uEa4H.js               19.90 kB
dist/assets/stancWorker-CJUPMKJw.js              1,943.28 kB
dist/assets/index-DAWNhffP.css                       2.51 kB │ gzip:   1.05 kB
dist/assets/mui-CgtEPZ7g.js                        322.76 kB │ gzip:  98.74 kB
dist/assets/utilities-C_bYLVjP.js                  324.77 kB │ gzip:  86.53 kB
dist/assets/index-Amr1Itqm.js                      362.06 kB │ gzip: 117.54 kB
dist/assets/plotly-cartesian-DY12K7b9.js         1,402.37 kB │ gzip: 478.09 kB

@WardBrian WardBrian requested a review from jsoules January 30, 2025 21:48
Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to clarify one of the comments, as I think people may be lost who haven't discussed the problem the way you and I have recently.

I will take you at your word about specifying the CDN URL, but is it really necessary to do so?

Comment on lines +112 to +115
const widgetPosition = {
position: { lineNumber: 1, column: 1 },
preference: [monacoInstance.editor.ContentWidgetPositionPreference.EXACT],
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this ought to be const-able, but the linter doesn't like it. Probably doesn't matter anyway...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow what you mean by const-able in this context?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean annotated as as const, since its members don't seem to be able to be changed anywhere in the code. But that conflicts with the looser typing further down.

(The larger context was me wondering if it should be memoized, before realizing this was already in a useEffect hook, so it doesn't matter.)

@jsoules
Copy link
Collaborator

jsoules commented Feb 3, 2025

(Noting that Monaco still loads in this version, and is noticeably faster to do so...)

@WardBrian
Copy link
Collaborator Author

I will take you at your word about specifying the CDN URL, but is it really necessary to do so?

Specifying the URL is necessary if you want control over the version. So far we have (unknowingly!) always been using monaco-loader's default version, which is 0.46

@jsoules
Copy link
Collaborator

jsoules commented Feb 3, 2025

Specifying the URL is necessary if you want control over the version. So far we have (unknowingly!) always been using monaco-loader's default version, which is 0.46

...weird. Wonder why they're so far behind...

@WardBrian
Copy link
Collaborator Author

From what I can tell, historically, requests for them to bump their default version are met with people being told just to use the config like this. I think they want to avoid doing an npm release every time monaco does one

@jsoules
Copy link
Collaborator

jsoules commented Feb 3, 2025

What must their release schedule be like, that that would be an actual problem greater than everybody by default using a version from 2023

@WardBrian
Copy link
Collaborator Author

Well, we were using a version from 2023 and didn't notice... ;P

@WardBrian WardBrian merged commit a4a408d into main Feb 3, 2025
2 checks passed
@WardBrian WardBrian deleted the monaco-import-fix branch February 3, 2025 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants