Bundle extension files using esbuild #1483

koesie10 · 2022-08-29T14:57:00Z

This bundles the extension files using esbuild, as recommended by VSCode. This reduces the size of the extension from 17MB to 6MB.

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

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

TODO

Integration tests are broken, probably because they are all using a different version of mocha.
This is quite a high-risk change since it changes how all files and modules are loaded.

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.

This bundles the extension files using esbuild, as recommended by VSCode. This reduces the size of the extension from 17MB to 6MB. Gulp will still run TypeScript to check types, but will not use the TypeScript compiler output in the bundle. See: https://code.visualstudio.com/api/working-with-extensions/bundling-extension

This disables the bundling of node_modules for the vscode-tests. This makes the test independent of each other but doesn't yet resolve all issues: proxyquire doesn't work and it's not possible to override some properties.

dbartol · 2022-08-29T15:11:58Z

If the TypeScript output is no longer being bundled, I assume that the bundled extension winds up using ts-loader or something like that to handle the TypeScript at runtime? How will our debug targets be affected? Will they also use ts-loader, or will they use precompiled TypeScript? I'd prefer that the debug targets be debugging something as close to the end-user environment as possible.

aeisenberg · 2022-08-29T15:20:04Z

extensions/ql-vscode/package.json

+  "__metadata": {
+    "id": "56d1e1f8-892a-4abc-b402-c473b832e324",
+    "publisherDisplayName": "GitHub",
+    "publisherId": "7c1c19cd-78eb-4dfb-8999-99caf7679002",
+    "isPreReleaseVersion": false


This looks generated. Is this required?

aeisenberg · 2022-08-29T15:23:42Z

If the TypeScript output is no longer being bundled, I assume that the bundled extension winds up using ts-loader or something like that to handle the TypeScript at runtime? How will our debug targets be affected? Will they also use ts-loader, or will they use precompiled TypeScript? I'd prefer that the debug targets be debugging something as close to the end-user environment as possible.

I have the same question...also, the extension is currently using source-map and source-map-support to get nice stack traces when errors are thrown during tests and at runtime. If typescript is no longer being compiled, then these two packages may no longer be necessary.

koesie10 · 2022-08-29T16:34:47Z

I'm sorry about the confusion, the PR body is not clear about this. The TypeScript files are still being compiled to JavaScript, but it's not using the TypeScript compiler anymore. ESBuild will take in the TypeScript files, strip the types, bundle it and create a .js and .js.map file.

Previously, the TypeScript compiler would create a .js file for every .ts file: extension.js, ide-server.ts, packaging.ts, etc. In combination with the node_modules directory, this would allow VSCode to run the extension.js file, which imports the other files and files in node_modules. However, this creates thousands of files, which can be quite slow to load.

In comparison, ESBuild will take in all the files in the extension and all node_modules and create 1 file: extension.js (and extension.js.map). This way, we don't need to copy node_modules anymore and are left with just this 1 file which VSCode needs to load, which can be much faster.

In terms of debugging, there will be no real difference since ESBuild will still generate a source map, so VSCode will automatically map the compiled bundle file to the original source paths (it might be using source-map/source-map-support, I'm not sure about that part). There is no difference in how the files are loaded in development or production, both will load just a single extension.js file.

Overall, this reduces the number of files from thousands (I believe 7,000+) down to 1 file which needs to be loaded by VSCode (or could potentially be loaded). In the current state of this PR, we are only including the node_modules/@vscode/codicons since that's used by the webview. However, this can potentially also be removed if we do #1482.

Unfortunately, we can't completely remove the node_modules folder. While it won't be included in the final bundled VSCode extension, we still require it for tests due to the setup of the tests. We might be able to get rid of it by restructuring the tests, but I don't think that's worth the time investment.

dbartol · 2022-08-29T17:02:02Z

There is no difference in how the files are loaded in development or production, both will load just a single extension.js file.

Great, this is what I was hoping for.

aeisenberg · 2022-08-29T17:03:46Z

Nice. Thanks for the explanation. This makes sense.

Looks like there's a failing build step for injecting the Application Insights key (for telemetry). The file that the script is looking for no longer exists since all files are concatenated, that step will need to change. The failure is here:

vscode-codeql/extensions/ql-vscode/gulpfile.ts/appInsights.ts

Line 14 in de6c523

return gulp.src(['out/telemetry.js'])

I think it would be sufficient to just change that to extension.js. The key it is replacing is unique enough that I don't think it will find any false positives.

Unfortunately, it's not easily possible to use ESBuild for the test setup. There are two reasons for this: 1. We can't use the bundled extension for the tests; the tests depend on multiple files existing and that the extension requires the files and modules (e.g. for proxyquire). 2. ESBuild compiles to ES6 modules which have additional restrictions on what can be modified. Therefore, it's not possible to set certain exported properties to mock them. The alternative implemented in this commit is to use a separate test directory for the tests. This directory is not bundled with the extension. Instead, this directory is only used for running the tests and contains the TypeScript compiler output instead of the bundled extension.

koesie10 · 2022-09-02T13:31:51Z

I've now got all the tests to work, but there is now quite a large difference between the tests and development/production. For development/production, the approach is as described as above: 1 bundled file is created which contains all extension files and required modules.

However, for the tests a completely different structure is used to facilitate mocking. If we use a bundled extension, we cannot (or only in a hacky way) access anything inside the extension. That would make it impossible to mock certain functions, which the test suite now uses quite extensively. Therefore, the extension is essentially build twice:

Development/production: ESBuild to bundle all files and node_modules
Testing: TypeScript compiler compiles all files to separate files and node_modules are copied

The differences between these environments is quite large (in terms of disk structure/how all modules interoperate), but they should function very similarly in terms of runtime behavior. There is still no difference between development/production, only between development/production and testing.

@dbartol @aeisenberg What are your thoughts on this? This is essentially the way that VSCode recommends setting up the tests, but we also need access to the internals of the extension, so we need two completely different builds. It still reduces the size of the extension from 23MB to 4.4MB (a reduction of just over 80%) and should also improve performance, especially on systems where IO is slow (I can image that IO is quite a bit slower on Windows).

aeisenberg

I'm a little concerned about using a vastly difference structure for running tests and for what we distribute. Doing so may mean that we don't catch extension packaging issues unless we test manually.

I mean...we should be testing manually anyway before a release because we have had other packaging issues. But these changes seem like it would be more likely for packaging bugs to slip through.

I think this change is very valuable since it helps make our published extension smaller and have fewer files. But I also think it is very valuable to (as much as possible) test what we ship. Can you be more explicit about what needs to change in order to run the tests from the esbuild extension?

My understanding is that the following is broken:

proxyquire
loading external test data

Anything else?

aeisenberg · 2022-09-02T15:58:00Z

extensions/ql-vscode/gulpfile.ts/appInsights.ts

@@ -11,7 +11,7 @@ export function injectAppInsightsKey() {
  }

  // replace the key
-  return gulp.src(['out/telemetry.js'])
+  return gulp.src(['out/extension.js'])


I haven't tested this myself. I would just like to make sure that you have verified that the APP_INSIGHTS_KEY env var is injected into the extension.js file appropriately.

aeisenberg · 2022-09-02T16:12:23Z

extensions/ql-vscode/gulpfile.ts/tests.ts

+
+function cleanTestsOutput() {
+  // The path to the test dist dir is outside of the working directory, so we need to force it
+  return del(testDistDir, { force: true });


Is del async?

aeisenberg · 2022-09-02T18:17:51Z

extensions/ql-vscode/gulpfile.ts/typescript.ts

-  return tsProject.src()
-    .pipe(tsProject(goodReporter()));
+  return tsProject.src();
+  // .pipe(tsProject(goodReporter()));


aeisenberg · 2022-09-02T18:22:16Z

extensions/ql-vscode/src/vscode-tests/no-workspace/contextual/queryResolver.test.ts

@@ -93,9 +94,9 @@ describe('queryResolver', () => {
    getPrimaryDbschemeSpy = sinon.stub();
    return proxyquire('../../../contextual/queryResolver', {
      'fs-extra': {
+        ensureDirSync,


On line 9, there is:

const proxyquire = pq.noPreserveCache().noCallThru();

I don't think noCallThru is necessary. If you remove that, do you still need to have this line?

aeisenberg · 2022-09-02T18:23:38Z

extensions/ql-vscode/src/vscode-tests/no-workspace/sarifParser.test.ts

@@ -1,6 +1,6 @@
 import * as path from 'path';
 import * as chai from 'chai';
-import chaiAsPromised from 'chai-as-promised';
+import * as chaiAsPromised from 'chai-as-promised';


I'm pretty sure this line and line 7 are not actually needed since chaiAsPromised is configured in extensions/ql-vscode/src/vscode-tests/no-workspace/index.ts.

koesie10 · 2022-09-05T14:56:31Z

My understanding is that the following is broken:

proxyquire

loading external test data
Anything else?

In general the problem is that we cannot access anything from the extension itself when running bundled tests. That also means that when a test exists to test it in the environment of VSCode, a different function will be tested than is actually loaded in the extension. For example, if we call sarifParser, we won't call the sarifParser function in the loaded extension, but a separate sarifParser function that is loaded as part of the test suite.

Loading external test data is actually fine since that doesn't involve us accessing the extension. Proxyquire is indeed unusable because when running a bundled test suite we're not loading any external modules.

I think it will be quite a challenge to solve this problem using our current test suite. The only safe way to test the extension is to call commands and retrieve the output from somewhere in VSCode. It is not safe to call a function inside of the extension. Some of our existing tests can be moved to the unit test suite which should not impact the actual functionality testing (I think the sarifParser test is safe to be run outside of VSCode). Others will be a lot harder, such as the cli-integration tests.

aeisenberg · 2022-09-06T17:09:10Z

Thanks for the explanation. That is indeed unfortunate. My concern is that we wind up testing something that is not being shipped and therefore miss something. I am not sure how likely this really is and I don't want to block what seems like a legitimate improvement in the extension.

Most likely, any problem that the automated tests miss would be around some sort of packaging and distribution change. If we were to commit to more manual (smoke) testing, or some level of end to end testing before releasing, I think this change would be fine.

koesie10 · 2022-11-25T14:12:25Z

Closing this in favour of #1799

koesie10 added 2 commits August 29, 2022 16:54

Don't bundle test node_modules

fc0d002

This disables the bundling of node_modules for the vscode-tests. This makes the test independent of each other but doesn't yet resolve all issues: proxyquire doesn't work and it's not possible to override some properties.

aeisenberg reviewed Aug 29, 2022

View reviewed changes

koesie10 added 8 commits August 31, 2022 16:17

Merge remote-tracking branch 'origin/main' into koesie10/bundle-files

ccf687a

Update app insights Gulp task for bundled extension

f2c9ef5

Setup testing setup

de509b5

Merge remote-tracking branch 'origin/main' into koesie10/bundle-files

28bf46d

Add back sourcemaps for test build

359ef0d

Use correct path for cli-integration test

724cc8e

Copy all data- directories for cli-integration tests

4c22e31

aeisenberg reviewed Sep 2, 2022

View reviewed changes

koesie10 closed this Nov 25, 2022

elenatanasoiu deleted the koesie10/bundle-files branch January 16, 2023 16:08

Bundle extension files using esbuild #1483

Bundle extension files using esbuild #1483

Uh oh!

Conversation

koesie10 commented Aug 29, 2022

TODO

Checklist

Uh oh!

dbartol commented Aug 29, 2022

Uh oh!

aeisenberg Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Aug 29, 2022

Uh oh!

koesie10 commented Aug 29, 2022

Uh oh!

dbartol commented Aug 29, 2022

Uh oh!

aeisenberg commented Aug 29, 2022

Uh oh!

koesie10 commented Sep 2, 2022

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

aeisenberg Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

aeisenberg Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

aeisenberg Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

aeisenberg Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

aeisenberg Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

koesie10 commented Sep 5, 2022

Uh oh!

aeisenberg commented Sep 6, 2022

Uh oh!

koesie10 commented Nov 25, 2022

Uh oh!

Uh oh!