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

Monaco offline support #2801

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Monaco offline support #2801

wants to merge 12 commits into from

Conversation

midigofrank
Copy link
Collaborator

@midigofrank midigofrank commented Jan 8, 2025

Description

This PR enables monaco editor to work offline. It configures the loader to look for the files in the assets/ directory.
This PR also creates another esbuild profile responsible for copying over the required monaco files. I've found this to be easier compared to configuring the worker: https://github.com/suren-atoyan/monaco-react/blob/master/README.md#loader-config

Closes #2786

Validation steps

Ensure you don't have any cache. You can test this on a private window

Run mix esbuild.install to get the newly configured version EDIT: No longer needed

  1. Run npm install --prefix assets to get the updated monaco editor. v0.43.0
  2. Turn off your internet
  3. Start the server and try editing any job's body

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

  • I have performed a self-review of my code.
  • I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

@midigofrank midigofrank self-assigned this Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.39%. Comparing base (9a54257) to head (dd4fca7).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2801   +/-   ##
=======================================
  Coverage   91.39%   91.39%           
=======================================
  Files         338      338           
  Lines       12052    12052           
=======================================
  Hits        11015    11015           
  Misses       1037     1037           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@midigofrank midigofrank marked this pull request as ready for review January 9, 2025 05:05
@josephjclark
Copy link
Contributor

@midigofrank when do we need to run mix esbuild.install? Do new users need to do this?

I ask because once this is merged to main, local editing will break for everyone, and they'll need to know to run that command

@midigofrank
Copy link
Collaborator Author

midigofrank commented Jan 9, 2025

@josephjclark mix esbuild.install needs to be run either way, even for new users. When you run mix setup, mix esbuild.install is run automagically for you.

Also, when you don't have the correct version, a warning is logged:

[warning] Outdated esbuild version. Expected 0.17.18, got 0.24.2. Please run `mix esbuild.install` or update the version in your config files.

EDIT:
For this PR once merged to main, they will have to run it because 0.17.18 doesn't support glob matching

@josephjclark
Copy link
Contributor

@midigofrank when I checked out this branch my monaco was dead. I'm sure there was a warning but there's SO MUCH logging that I didn't see it.

mix setup isn't mentioned at all in the RUNNINGLOCAL.md file 🤔 Is that handled by some other magic?

Is there any other way we can alert users about this change?

@josephjclark
Copy link
Contributor

Oh @midigofrank I have an unchecked in mix.lock file after running the esbuild install - maybe that's the issue?

@midigofrank
Copy link
Collaborator Author

Err, unchecked mix.lock is fine.

I'm surprised that mix setup isn't mentioned anywhere in RUNNINGLOCAL.md. Let me review the getting started guide and see how it's solved.

This is a non issue for those running Docker though

@midigofrank
Copy link
Collaborator Author

Also, for new users we are kinda safe because the assets won't really be built unless esbuild is installed. I reckon the app won't even start if it's missing.

@josephjclark
Copy link
Contributor

Ok, so it it's just our dev team that's going to hurt, we don't need to worry too much. Maybe we can add a troubleshooting line somewhere just as a fallback?

@josephjclark
Copy link
Contributor

@midigofrank the other thing I noticed here is that the file sizes of the monaco build have bloated significantly

Here's local dev on main (prod looks about the same):

monaco-main

Here's the same files on this

monaco-branch
branch:

Note that editor.main.js is up to 18mb and tsWrorker.js is 17mb.

I've had a quick look at the worker file and it's not obvious to me what the difference is. The new file is many many more lines, but when I skim through it seems to have the same formatting and contents. Weird. I note that you're loading the min/vs folder so you should be doing the right thing 🤔

I don't have time to investigate this any further right now - are you able to take a look?

@midigofrank
Copy link
Collaborator Author

Whoa!! Good catch. Let me try and sift through

@midigofrank
Copy link
Collaborator Author

It seems the problem was that they weren't getting minified. In production we add the flag --minify. Here is the result, not as good as the cdn though:
Screenshot 2025-01-09 at 16 47 58

@josephjclark
Copy link
Contributor

Huge Improvement, thank you @midigofrank !

@josephjclark
Copy link
Contributor

Hmm, hang on 🤔

The unminified code in dev, on main and branch, is very different. Neither is minified in dev right? Doesn't need to be.

But there's a lot more content coming in the new version

That'll be why the files still arent as small as the CDN build. It's not the minification, it's the content

Those prod sizes are acceptable I think but I'd still like to understand the difference

@midigofrank
Copy link
Collaborator Author

Hmm, yeah it could be the difference in versions. In staging and prod, the cdn is pointing to 0.43.0. Locally, I have 0.34.1.

@josephjclark
Copy link
Contributor

@midigofrank ooh that's interesting. A very likely culprit. And scary! It's critical that local dev and prod are running the same versions of software.

Can you work out how to lock down those versions to 0.43.0?

@midigofrank
Copy link
Collaborator Author

Ooh and local dev on main with the cdn, is also pointing to 0.43.0

It seems we just need to lock monaco-editor to 0.43.0 as well

@midigofrank
Copy link
Collaborator Author

Okay, 0.43.0 is actually bigger in size:
Screenshot 2025-01-09 at 17 46 43

@midigofrank
Copy link
Collaborator Author

I have upgraded it to 0.43.0. This came with a regression. I could no longer just run esbuild --minify on the lightning bundle, I have been forced to specify the platform --platform=node.
Screenshot 2025-01-09 at 18 24 13

@josephjclark
Copy link
Contributor

Thanks @midigofrank

I've just run through to check a bunch of things:

  • Production right now uses monaco 0.43.0 from jsdeliver. The tsworker is 1.1mb on the wire. Source looks minified (although the typescript defs are not)
  • here it is!
  • Dev right now uses exactly the same file, 0.43.0
  • Dev is about 4mb on the wire

Both tsworker files are about 4mb on disk.

So basically, the difference between the CDN file and the local file is better compression over the wire through some CDN magic.

Copy link
Contributor

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

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

@midigofrank
Copy link
Collaborator Author

midigofrank commented Jan 10, 2025

Huh!! I have run mix phx.digest and this is the result. The prod Dokerfile doesn't run this command 🤷 .
This PR shouldn't get merged until this is clarified

EDIT: It is actually being run when you run mix assets.deploy
Screenshot 2025-01-10 at 08 46 38

@midigofrank
Copy link
Collaborator Author

Hey @josephjclark , this is a problem. mix assets.deploy which is used in prod fails at this point. With the upgrade of monaco to 0.43.0, there is a package @typescript/vfs which is confused as to where to find path. This is why it was suggesting to use node as the target platform.

Not sure how to proceed here

I have upgraded it to 0.43.0. This came with a regression. I could no longer just run esbuild --minify on the lightning bundle, I have been forced to specify the platform --platform=node. Screenshot 2025-01-09 at 18 24 13

@josephjclark
Copy link
Contributor

Hey @midigofrank I'll take a look later. Remember that prod is actually on 0.43 so it's not the version change, it's the build change generally

@josephjclark
Copy link
Contributor

Call summary:

  • the actual problem here is the esbuild version bump, which breaks the standard esbuild profile
  • I think this is caused by esbuild 22 (Click for release notes). One day this is going to bite us in the unmentionables
  • But not today. We don't actually need esbuild to process the monaco files - we can just do a simple cp from assets/node_modules/... to priv/assets (or whatever) and serve the minified monaco files directly

@theroinaochieng
Copy link
Contributor

@josephjclark thanks for flagging this, I've noted your concerns about potential future issues related to esbuild 22, dropped a note to understand how the implications of this release so we can figure out scope and prioritization

@midigofrank I can't seem to see any notes for QA on this issue description, do you mind adding them? Also reached out about who our second reviewer on this should be. Major claps for finishing this!

@midigofrank
Copy link
Collaborator Author

@josephjclark after several deliberations I have found a way through with just esbuild. Given that esbuild 0.17 doesn't support glob matching, all I have done is expanded the glob from elixir's end.

Using cp is indeed simple but in the grand scheme of things, it means we have 2 separate ways of building the asset pipeline and I was heavily trying to avoid that.

Copy link
Member

@taylordowns2000 taylordowns2000 left a comment

Choose a reason for hiding this comment

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

Hey @midigofrank , I got a review request here. (CC @theroinaochieng .)

I've just attempted the validation steps but my Monaco is dead:
image

[info] GET /assets/monaco-editor/vs/loader.js
[debug] ** (Phoenix.Router.NoRouteError) no route found for GET /assets/monaco-editor/vs/loader.js (LightningWeb.Router)
    (lightning 2.10.9) deps/phoenix/lib/phoenix/router.ex:541: LightningWeb.Router.call/2
    (lightning 2.10.9) lib/lightning_web/endpoint.ex:1: LightningWeb.Endpoint.plug_builder_call/2
    (lightning 2.10.9) lib/lightning_web/endpoint.ex:1: LightningWeb.Endpoint."call (overridable 3)"/2
    (lightning 2.10.9) deps/plug/lib/plug/debugger.ex:136: LightningWeb.Endpoint."call (overridable 4)"/2
    (lightning 2.10.9) lib/lightning_web/endpoint.ex:1: LightningWeb.Endpoint.call/2
    (phoenix 1.7.18) lib/phoenix/endpoint/sync_code_reload_plug.ex:22: Phoenix.Endpoint.SyncCodeReloadPlug.do_call/4
    (plug_cowboy 2.7.2) lib/plug/cowboy/handler.ex:11: Plug.Cowboy.Handler.init/2
    (cowboy 2.12.0) /Users/taylor/build/lightning/deps/cowboy/src/cowboy_handler.erl:37: :cowboy_handler.execute/2
    (cowboy 2.12.0) /Users/taylor/build/lightning/deps/cowboy/src/cowboy_stream_h.erl:306: :cowboy_stream_h.execute/3
    (cowboy 2.12.0) /Users/taylor/build/lightning/deps/cowboy/src/cowboy_stream_h.erl:295: :cowboy_stream_h.request_process/3
    (stdlib 5.2.3) proc_lib.erl:241: :proc_lib.init_p_do_apply/3

I also see an approval from Joe, but what sounds like an open conversation. Can you only add me back as a reviewer once this thing is ready to test out?

Or, if it is ready to test, please drop in some validation instructions that replace this current set of instructions:

Ensure you don't have any cache. You can test this on a private window

  1. Run mix esbuild.install to get the newly configured version
  2. Turn off your internet
  3. Start the server and try editing any job's body

As a separate bit... I'm worried that there's now a RUNNINGLOCAL.md file. I see this was created 4 months back, and I'm sorry for not noticing and speaking up then, but why does this exist and duplicate so much of the official https://openfn.github.io/lightning/readme.html#dev-on-lightning-locally instruction?

Conclusion

Two things to do here:

  1. Let me know if/when this is ready for review. (If now, provide updated review instructions.)
  2. Delete RUNNINGLOCAL.md or... justify its existence, remove duplication from the README.md, and link to it from the README.md

Thank you! This is awesome work and I know MSF will be delighted to see this in action.

@theroinaochieng
Copy link
Contributor

@taylordowns2000 thank you for this feedback. I'll circle back with Frank and re-request a review when the provided actions are completed.

@midigofrank
Copy link
Collaborator Author

Hey @taylordowns2000 I have just pushed the fix for the issue you were experiencing, sorry my bad, I've also updated the validation instructions accordingly.

As for RUNNINGLOCAL.md, it is separate because there was need for a guide that can help a dev setup and run lightning locally. Yes, most of it is duplicated from the readme but the whole idea was to declutter the README

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

Get Monaco working fully offline
5 participants