web: Fix settings editor completion/validation#60808
Conversation
5bc30ac to
ce0129e
Compare
a86d8e4 to
07472f9
Compare
There was a problem hiding this comment.
Without this there would be a client side error. This is needed to show a tooltip when hovering over warnings and errors.
There was a problem hiding this comment.
This was moved and adapted from manifest.test.ts.
|
@fkling This looks great! Unfortunately I'll do a quick build with this to try and see if the bazel bundle works. What page / route can I nav to to test the editor? |
|
@jamesmcnamara You can go to
That's what I feared... as you can see there was some bazel specific logic in the manifest plugin that seemed unnecessary, it would be great to verify somehow that the files can actually be loaded in a complete bazel build. |
|
I'm gonna try adding a target to run an esbuild server out of the bazel output tree; could be useful for other local flows. |
|
@fkling I sank most of the day into trying to reproduce the bazel issue without any real luck. I even built a frontend image off of main and then used the https://github.com/sourcegraph/deploy-sourcegraph-docker config to bring a whole sourcegraph in docker and they all had code completion in the settings editor. So potentially this is a dotcom/S2 specific issue? It's kind of strange |
|
@jamesmcnamara That is strange indeed. Honestly I don't know if it's broken only for dotcom and S2. It seems weird that it would only happen for those two. If you go to user settings on dotcom or S2 you can see that it fails to load the worker files. I don't know where these files would come from... Actually, taking a step back: The production builds we ship to dotcom and S2 are full bazel builds? Or are we not using the web bazel targets for production? |
|
@fkling When I go to S2 or dotcom I see that it hits an error on When I build I believe what we ship in production is the I'd be happy to pair on this if you have any ideas but I'm unfortunately a little uninformed of the dark depths of how dotcom/s2 are released. |
|
Good news is that it wasn't a weird S2 issue! Somehow along the way I managed to merge your changes into my local main so when I was building it through bazel it was actually building your code and not main. Thus it was actually confirming that this code works. I added a target to sg.config One thing that is still weird is that the working code still throws that above error from the monaco code (I assume because that file didn't get bundled into CJS?). It doesn't seem to be breaking anything though. Here's a video JH took proving it works locally through Bazel. |
|
@jamesmcnamara yay! Thank you for confirming 😃 I'll try to find out what's up with the foreign module error. |
|
@jamesmcnamara FWIW I don't see that foreign module error when running it locally (non-bazel). Do you see that only with bazel? |
eseliger
left a comment
There was a problem hiding this comment.
wow! thank you so much, that's definitely not been a simple fix. ty for treading through it :)
There was a problem hiding this comment.
@jamesmcnamara Added them for testing the bazel frontend build. I can remove them again if we just want to focus on fixing the issue.
e802550 to
2ef808b
Compare
It doesn't seem to be needed anymore. There are no `worker.*` files/imports in this directory and `pnpm build` works without the plugin.
2ef808b to
72209ea
Compare
Fixes #60492
Completions and validation are not working in the settings editor. This is a huge problem because it (a) makes settings much more difficult to use (users don't know which settings exists or what they do) and (b) can lead to invalid settings configurations.
Problem
Completion and validation is provided by web workers, which are not available in production. The files don't exist:
This seems to have become an issue when we switched to bazel production builds. There is no rule for building those workers.
Solution(s)
I tried various approaches, which is why this PR contains multiple related changes.
Attempt 1: Additional entry files for workers
I realized that completion and validation worked fine in development builds, which doesn't use bazel. There we kick off a separate esbuild process to bundle the worker files expected by Monaco.
I wanted to avoid adding a new esbuild target to bazel. I thought if these two processes are decoupled we might run into similar problems in the future.
So instead I looked into adding additional entry points to the main esbuild config file which is also used by bazel.
This turned out to be more difficult than expected because the
manifestPluginonly worked for the very specific setup of having anmainentry point and anembedentry point, and nothing else.I refactored the manifest generation logic to make it work better with different entry point configurations.
However, in the end I had to give up on that approach because it's apparently not possible to have different output file patterns for different entry points. The current config appends the content hash to the
mainandembedoutput files, but the worker files would need a static/deterministic file name so that we can reference them directly from code. esbuild doesn't seem to offer that flexibility (all output file names contained the content hash).Even though this approach didn't work out, I've kept the changes to the manifest logic to make it more resilient to future changes.
Attempt 2: Inline workers
I still wanted all this to be a single build process, so my next attempt was to create "inline workers" with the existing
workerPlugin, and directly return a worker to Monaco, instead of a worker URL. I had to make a couple of adjustments to the worker plugin to make it work with workers imported from node modules.There are also issues with TypeScript and the Monaco API and worker modules, which forced me to use
@ts-ignorein three places (all documented).Test plan
sg start entrprise-bazelbut I don't know if that actually uses the bazel version of the web app.Todo
I removed the plugin.
pnpm buildstill works without it and there don't seem to be any imports of worker files in this directory.