Bundle extension files using ESBuild

[koesie10]

This bundles the extension files using esbuild, as recommended by VSCode. This reduces the size of the extension from 34MB to less than 5MB.

Gulp will still run TypeScript to check types, but will not use the TypeScript compiler output in the bundle.

Tests are now run separately, outside of Gulp, so their data doesn't need to be copied anymore.

See: https://code.visualstudio.com/api/working-with-extensions/bundling-extension

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[koesie10]

I think this PR is now ready for review. All tests are passing. I haven't run through the full test plan, so we should still do that before merging this. I've also created #1911 which uses Webpack instead of ESBuild. See the internal linked issue for some more information about the differences.

[charisk]

Great work @koesie10!

Agree we should run a thorough test of the various features (ideally also ones that aren't covered by our test plan yet). Perhaps we can ask for some help from others.

One thing that isn't working for me yet is the ability to debug the extension e.g. put a breakpoint and execute some code that would hit the breakpoint. At the minute, the breakpoints don't bind.

[koesie10]

One thing that isn't working for me yet is the ability to debug the extension e.g. put a breakpoint and execute some code that would hit the breakpoint. At the minute, the breakpoints don't bind.

Can you please try again? There was a missing setting in the ESBuild config which caused VSCode not to be able to map the sourcemap to the actual source code. That should have been fixed now and I'm able to hit breakpoints.

[charisk]

One thing that isn't working for me yet is the ability to debug the extension e.g. put a breakpoint and execute some code that would hit the breakpoint. At the minute, the breakpoints don't bind.

Can you please try again? There was a missing setting in the ESBuild config which caused VSCode not to be able to map the sourcemap to the actual source code. That should have been fixed now and I'm able to hit breakpoints.

Thanks, I can hit breakpoints now!

[charisk]

Can you explain why we're removing fs mocking e.g. in
0d8c90a

[koesie10]

Can you explain why we're removing fs mocking e.g. in 0d8c90a

ESBuild uses a slightly different method of importing dependencies which doesn't allow redefining exported methods, so you get errors like TypeError: Cannot redefine property: writeFile at Function.defineProperty (<anonymous>) when calling jest.spyOn on modules. The different behavior is probably because we need to set esModuleInterop in the tsconfig.json, so it also affects the tests.

I think in terms of speed it shouldn't matter whether we're mocking or writing the actual files, but I think writing the files to disk will make the tests slightly better at spotting errors. We're also not just overwriting an exists call, but now also check that we're checking the correct file. What do you think?

[charisk]

Can you explain why we're removing fs mocking e.g. in 0d8c90a

ESBuild uses a slightly different method of importing dependencies which doesn't allow redefining exported methods, so you get errors like TypeError: Cannot redefine property: writeFile at Function.defineProperty (<anonymous>) when calling jest.spyOn on modules. The different behavior is probably because we need to set esModuleInterop in the tsconfig.json, so it also affects the tests.

I think in terms of speed it shouldn't matter whether we're mocking or writing the actual files, but I think writing the files to disk will make the tests slightly better at spotting errors. We're also not just overwriting an exists call, but now also check that we're checking the correct file. What do you think?

I also prefer not mocking fs, but I wanted to understand why this change was relevant to bundling. Thanks for explaining!

[charisk]

Thanks for getting this sorted and dealing with all the merge conflicts etc.!

The PR looks good to me, and the extension seems to work okay with the changes, but I've not done any extensive testing.

It would be great to also get a review from @aeisenberg or someone else if possible.

[aeisenberg]

Nice work! Looks generally good to me. I have one question, and after that's addressed, 🚢.

[aeisenberg]

Nice, but just want to be clear that this is still all you need to do for watching things. In particular, it looks like watchTestData is not being used any more. Is it still working as expected?

[koesie10]

The tests are now run from their source code directly, so we don't need to copy the test data to out anymore. I believe we could also have combined all these different invocations of gulp to a single invocation to gulp watch in the previous setup, but now that we don't have a separate tsc --watch step anymore, it makes even more sense.

[aeisenberg]

Thanks. This makes sense. Nice improvement.

[charisk]

Does debugging integration tests still work for you @koesie10?

[koesie10]

Does debugging integration tests still work for you @koesie10?

Yes, debugging the integration tests using Jest Runner still seems to work after I uncomment the settings in settings.json.

[charisk]

Does debugging integration tests still work for you @koesie10?

Yes, debugging the integration tests using Jest Runner still seems to work after I uncomment the settings in settings.json.

Sorry which settings are those? So far I've been able to just click the Debug button

[koesie10]

Sorry which settings are those? So far I've been able to just click the Debug button

Those are these settings:

diff --git a/.vscode/settings.json b/.vscode/settings.json
index 984ccf8c..c70a42f0 100644
--- a/.vscode/settings.json
+++ b/.vscode/settings.json
@@ -44,12 +44,12 @@
   },
   "jestrunner.debugOptions": {
     // Uncomment to debug integration tests
-    // "attachSimplePort": 9223,
+    "attachSimplePort": 9223,
     "env": {
       "LANG": "en-US",
       "TZ": "UTC",
       // Uncomment to debug integration tests
-      // "VSCODE_WAIT_FOR_DEBUGGER": "true",
+      "VSCODE_WAIT_FOR_DEBUGGER": "true",
     }
   },
   "terminal.integrated.env.linux": {

This is also mentioned in the CONTRIBUTING file, although it's not explicitly called out, only mentioned.

[charisk]

Sorry which settings are those? So far I've been able to just click the Debug button

Those are these settings:

diff --git a/.vscode/settings.json b/.vscode/settings.json
index 984ccf8c..c70a42f0 100644
--- a/.vscode/settings.json
+++ b/.vscode/settings.json
@@ -44,12 +44,12 @@
   },
   "jestrunner.debugOptions": {
     // Uncomment to debug integration tests
-    // "attachSimplePort": 9223,
+    "attachSimplePort": 9223,
     "env": {
       "LANG": "en-US",
       "TZ": "UTC",
       // Uncomment to debug integration tests
-      // "VSCODE_WAIT_FOR_DEBUGGER": "true",
+      "VSCODE_WAIT_FOR_DEBUGGER": "true",
     }
   },
   "terminal.integrated.env.linux": {

This is also mentioned in the CONTRIBUTING file, although it's not explicitly called out, only mentioned.

Weird, I never had to do that before. Works with that change though, thanks!

[charisk]

FWIW I tested out a few things:

• Running a MRVA
• Running a local query
• Debugging the extension
• Debugging unit/integration tests
• A bit of poking in the query history

[aeisenberg]

Hmmmm...I'm having trouble debugging the cli integration tests.

Here is my .vscode/settings.json:

"jestrunner.debugOptions": {
    // Uncomment to debug integration tests
    "attachSimplePort": 9223,
    "env": {
      "LANG": "en-US",
      "TZ": "UTC",
      // Uncomment to debug integration tests
      "VSCODE_WAIT_FOR_DEBUGGER": "true",
    }
  },

I am clicking on the debug code lens in variant-analysis-manager.test.ts "should run a remote query that is nested inside a qlpack" in using the vscode-jest-runner extension. The test process doesn't seem to be able to attach to vscode and the test is never run. The jest process runs in a loop and times out. Here is a sample of the output I am seeing:

Shell output from jest run

$ cd /Users/andrew.eisenberg/Eclipse/git-repos/github/vscode-codeql/extensions/ql-vscode ; /usr/bin/env LANG=en-US TZ=UTC VSCODE_WAIT_FOR_DEBUGGER=true 'VSCODE_INSPECTOR_OPTIONS={"inspectorIpc":"/var/folders/41/kxmfbgxj40dd2l_x63x9fw7c0000gn/T/node-cdp.52486-8.sock","deferredMode":false,"waitForDebugger":"","execPath":"/Users/andrew.eisenberg/.nvm/versions/node/v16.18.1/bin/node","onlyEntrypoint":false,"autoAttachMode":"always","fileCallback":"/var/folders/41/kxmfbgxj40dd2l_x63x9fw7c0000gn/T/node-debug-callback-6ce3e3fe600e3372"}' /Users/andrew.eisenberg/.nvm/versions/node/v16.18.1/bin/node node_modules/.bin/jest /Users/andrew.eisenberg/Eclipse/git-repos/github/vscode-codeql/extensions/ql-vscode/test/vscode-tests/cli-integration/remote-queries/variant-analysis-manager.test.ts -c /Users/andrew.eisenberg/Eclipse/git-repos/github/vscode-codeql/extensions/ql-vscode/test/vscode-tests/cli-integration/jest.config.ts -t Variant\ Analysis\ Manager\ runVariantAnalysis\ should\ run\ a\ remote\ query\ that\ is\ nested\ inside\ a\ qlpack --runInBand Found existing install in /Users/andrew.eisenberg/Eclipse/git-repos/github/vscode-codeql/extensions/ql-vscode/.vscode-test/vscode-darwin-1.74.3. Skipping download Installing required extensions for /Users/andrew.eisenberg/Eclipse/git-repos/github/vscode-codeql/extensions/ql-vscode/.vscode-test/vscode-darwin-1.74.3/Visual Studio Code.app/Contents/MacOS/Electron

Installing extensions...
Extension 'hbenl.vscode-test-explorer' v2.21.1 is already installed. Use '--force' option to update to latest version or provide '@' to install a specific version, for example: 'hbenl.vscode-test-explorer@1.2.3'.
Extension 'ms-vscode.test-adapter-converter' v0.1.6 is already installed. Use '--force' option to update to latest version or provide '@' to install a specific version, forCLI version v2.11.6 is found /Users/andrew.eisenberg/Eclipse/git-repos/github/vscode-codeql/extensions/ql-vscode/build/cli/v2.11.6/codeql/codeql. Not going to download again.
Found existing install in /Users/andrew.eisenberg/Eclipse/git-repos/github/vscode-codeql/extensions/ql-vscode/.vscode-test/vscode-darwin-1.74.3. Skipping download
[main 2023-01-12T19:33:00.748Z] update#setState idle
2023-01-12 19:33:02.009 Code Helper (Renderer)[9004:8386303] CoreText note: Client requested name ".NewYork-Regular", it will get Times-Roman rather than the intended font. All system UI font access should be through proper APIs such as CTFontCreateUIFontForLanguage() or +[NSFont systemFontOfSize:].
2023-01-12 19:33:02.009 Code Helper (Renderer)[9004:8386303] CoreText note: Set a breakpoint on CTFontLogSystemFontNameRequest to debug.
[main 2023-01-12T19:33:02.496Z] Starting extension host with pid 9830 (fork() took 2 ms).
Loading development extension at /Users/andrew.eisenberg/Eclipse/git-repos/github/vscode-codeql/extensions/ql-vscode
[LocalProcessExtensionHost]: Extension host did not start in 10 seconds (debugBrk: true)
[main 2023-01-12T19:33:30.755Z] update#setState checking for updates
[main 2023-01-12T19:33:31.036Z] update#setState idle
Error received from starting extension host (kind: LocalProcess)
The local extension host took longer than 60s to connect.
[main 2023-01-12T19:34:02.713Z] Waiting for extension host with pid 9830 to exit.
[main 2023-01-12T19:34:08.717Z] Extension host with pid 9830 did not exit within 6000ms.
[main 2023-01-12T19:34:08.726Z] Extension host with pid 9830 exited with code: null, signal: SIGTERM.
VS Code exited with exit code 1
[main 2023-01-12T19:34:09.721Z] update#setState idle
2023-01-12 19:34:10.576 Code Helper (Renderer)[10692:8390185] CoreText note: Client requested name ".NewYork-Regular", it will get Times-Roman rather than the intended font. All system UI font access should be through proper APIs such as CTFontCreateUIFontForLanguage() or +[NSFont systemFontOfSize:].
2023-01-12 19:34:10.576 Code Helper (Renderer)[10692:8390185] CoreText note: Set a breakpoint on CTFontLogSystemFontNameRequest to debug.
Loading development extension at /Users/andrew.eisenberg/Eclipse/git-repos/github/vscode-codeql/extensions/ql-vscode
[main 2023-01-12T19:34:11.574Z] UtilityProcess<1>: Creating new...
[main 2023-01-12T19:34:11.578Z] UtilityProcess<1>: received spawn event.

Some other things I noticed:

1. I think we need to copy the extra env vars from here to here so that it will be easy to choose different CLI versions and codeql checkouts to run the tests with.
2. When I run non-cli tests, eg here, I can run and debug (yay!). However, for some reason, the test executes twice.

I'm pretty sure I get the same behaviour when running on main (so it's not this PR, but rather the changes to jest that introduced this weirdness). It's also highly possible there is something strange with my setup if no one else is seeing it.

[aeisenberg]

Given that the difficulties running and debugging tests are already in main, I think this is fine to go through. But, maybe we can pair at some point to look into debugging some more?

[aeisenberg]

I also tried building the extension and installing it into my vscode. Works perfectly.

[koesie10]

Thanks for the reviews @charisk and @aeisenberg! I've tested the extension myself as well and haven't found any features which aren't working. Since you both also haven't found anything that isn't working, I'll go ahead and merge this.

Given that the difficulties running and debugging tests are already in main, I think this is fine to go through. But, maybe we can pair at some point to look into debugging some more?

Sure! I don't actually use the debugging features that much, so that's probably why I missed that these things are not working.