github
Bundle extension files using esbuild
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-reviewlabel there.
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.
If the TypeScript output is no longer being bundled, I assume that the bundled extension winds up using
ts-loaderor something like that to handle the TypeScript at runtime? How will our debug targets be affected? Will they also usets-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.
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.
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.
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: https://github.com/github/vscode-codeql/blob/de6c523bad8b3383b05c722a4682f0066b3b7097/extensions/ql-vscode/gulpfile.ts/appInsights.ts#L14
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.
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_modulesare 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).
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.
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.