search

microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.14k stars 29.28k forks source link

Explore AMD to ESM migration #160416

Closed jrieken closed 2 months ago

jrieken commented 2 years ago

The VS Code project uses AMD (asynchronous module definition) which is somewhat of a pre-runner of native JS modules (ESM). AMD is mostly dead technology and we should migrate away from it. Our TypeScript sources are written with ESM imports/exports but we took a few dependencies on AMD as a "runtime" and we need to understand what it takes to move towards ESM.

High-level topics:

Backlog Plan: https://github.com/microsoft/vscode/issues/160416#issuecomment-2269225658

Test Builds that run on ESM:

VS Code Build: 7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d

darwin: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/VSCode-darwin.zip
darwin-arm64: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/VSCode-darwin-arm64.zip
darwin-universal: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/VSCode-darwin-universal.zip
linux-arm64: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/code-exploration-arm64-1723616523.tar.gz
linux-deb-arm64: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/code-exploration_1.93.0-1723616603_arm64.deb
linux-deb-x64: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/code-exploration_1.93.0-1723617891_amd64.deb
linux-rpm-arm64: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/code-exploration-1.93.0-1723616690.el8.aarch64.rpm
linux-rpm-x64: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/code-exploration-1.93.0-1723617981.el8.x86_64.rpm
linux-snap-x64: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/code-exploration-x64-1723619959.snap
linux-x64: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/code-exploration-x64-1723616784.tar.gz
win32-arm64-user: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/VSCodeUserSetup-arm64-1.93.0-exploration.exe
win32-x64-user: https://vscode.download.prss.microsoft.com/dbazure/download/exploration/7c7401b2553c7188f1a634b5264fc3a6d3cb1e7d/VSCodeUserSetup-x64-1.93.0-exploration.exe
lorand-horvath commented 2 years ago

Huh, VSCode still using AMD...

jasonwilliams commented 1 year ago

Hey @jrieken where would this leave extensions? Would there be a path for them to export esm? I believe right now they still export to commonjs because electron does not support esm

jrieken commented 1 year ago

Hey @jrieken where would this leave extensions? Would there be a path for them to export esm? I believe right now they still export to commonjs because electron does not support esm

@jasonwilliams For now extensions aren't affected by this and for the time being commonjs is how they roll. Tho, I am learning a ton and there will be a path for ESM extensions. We ignore this to keep this already large effort smaller. Also, if you wonder why this is complex: when extensions call require('vscode') something very special happens, there is no file for vscode but each extension gets an instance of a the API. This is only possible with commonjs loader tricks and we need to find similar ways in the ESM world.

jasonwilliams commented 1 year ago

@jrieken thanks for your response on this, looking forward to seeing what solution you come up with. I agree keeping extensions out makes things a lot easier.

Also, if you wonder why this is complex: when extensions call require('vscode') something very special happens, there is no file for vscode but each extension gets an instance of a the API. This is only possible with commonjs loader tricks and we need to find similar ways in the ESM world.

Would import maps not help with this? Extensions already don’t bundle “vscode” and leave it as an external reference. I assume once Code has launched its extensions can be mapped to a specific location for the VSCode module. Anything more programmatic than that may take a while to be standarized.

jrieken commented 1 year ago

Yeah, import maps is our only viable option for this. Fingers crossed for Safari landing this in time for us

zm-cttae commented 1 year ago

If Safari doesn't support this, is core team blocked by the few months it takes for browser updates to percolate to userland?

See Google Chrome 110 popularity vs Google Chrome 109 here: https://www.stetic.com/market-share/browser/

jasonwilliams commented 1 year ago

Looks like Safari have included import maps support in their latest release https://developer.apple.com/documentation/safari-release-notes/safari-16_4-release-notes

jasonwilliams commented 1 year ago

@jrieken I am curious how you are going to achieve this when Electron doesn't support ESM. https://github.com/electron/electron/issues/21457

Is this something you've considered?

zm-cttae commented 1 year ago

Doesn't Node 16.14.2 support it? EDIT: they need new dependency for Node import maps

Jack-Works commented 1 year ago

https://github.com/guybedford/es-module-shims supports import map polyfill

JustinGrote commented 1 year ago

Electron now has support for this so the question will be when the newest Electron gets merged into vscode image

SimonSiefke commented 11 months ago

@jrieken if it's fine with you, I could work on moving the build folder to esm.

Perhaps after that, some of the workers (textMateTokenizationWorker, outputLinkComputer, editorWorkerService) could be moved to esm in three steps:

  1. For each of the workers above, Instead of creating a Classic-Worker, create a Module-Worker
  2. Change simpleWorker so that it can not only require, but also import another file
  3. Change the build process so that the final textMateTokenizationWorker, outputLinkComputer and editorWorkerService files are esm
jrieken commented 11 months ago

@jrieken if it's fine with you, I could work on moving the build folder to esm.

Thanks for the offer but migration the build folder isn't on this plan. It uses commonjs, not AMD, and hence different cup of tea. Not saying it shouldn't be done but it's nothing pressing and would need team support first.

jasonwilliams commented 9 months ago

Hey @jrieken would love to know if you have any update at on this. It sounds like all the building blocks you need to make this happen have arrived. (Safari supporting import maps, Electron supporting ESM), it feels like there shouldn’t be any blockers to making this a reality now.

JustinGrote commented 9 months ago

@jasonwilliams The Electron version vscode uses still has to reach the version that supports ESM, that's a ways away still.

JustinGrote commented 7 months ago

As an update, the electron version is now on an ESM-supportable version, but the loader still needs to be fixed to enable it. This is no longer an external dependency now however.

https://github.com/microsoft/vscode-loader/issues/56

metawrap-dev commented 7 months ago

I will be very glad when this is resolved. I have a component of a project on hold until this can be resolved.

jrieken commented 4 months ago

Things for July

bpasero commented 4 months ago

yarn test-node works, but shows some interesting few errors when loading jschardet:

  3784 passing (8s)
  40 pending
  5 failing
1) Encoding
   autoGuessEncoding (UTF8):

TypeError: Cannot read properties of undefined (reading 'iterator')
      at $jscomp.initSymbolIterator (evalmachine.<anonymous>:1:384)
      at $jscomp.makeIterator (evalmachine.<anonymous>:2:42)
      at evalmachine.<anonymous>:164:334
      at new c (evalmachine.<anonymous>:164:465)
      at evalmachine.<anonymous>:662:132
      at 42../constants (evalmachine.<anonymous>:662:395)
      at f (evalmachine.<anonymous>:5:32)
      at evalmachine.<anonymous>:5:66
      at 19../logger (evalmachine.<anonymous>:390:112)
      at f (evalmachine.<anonymous>:5:32)
      at evalmachine.<anonymous>:5:66
      at 1../src (evalmachine.<anonymous>:5:249)
      at f (evalmachine.<anonymous>:5:32)
      at a (evalmachine.<anonymous>:5:190)
      at DefineCall.callback (evalmachine.<anonymous>:5:218)
      at AMDModuleImporter.load (file:///Users/bpasero/Development/Microsoft/vscode-esm/out/vs/amdX.js:79:31)
SimonSiefke commented 4 months ago

@bpasero I encountered the same error. It seems the issue is that this is undefined at the top level of Ecmascript modules:

In jschardet.min.js:

this

var $jscomp = { scope: {}, global: this }, // this is undefined
    Symbol;
$jscomp.initSymbol = function () {
    $jscomp.global.Symbol || (Symbol = $jscomp.Symbol); // error occurs here
    $jscomp.initSymbol = function () {};
};

When accessing $jscomp.global.Symbol it is accessing undefined.Symbol.

There is a PR here https://github.com/microsoft/vscode-loader/pull/58 that might fix it.

bpasero commented 4 months ago

Thanks for the insights, maybe I am missing something but in the alex/esm (https://github.com/microsoft/vscode/pull/166033) branch where our work happens, we do not use the vscode-loader anymore I had thought 🤔

SimonSiefke commented 4 months ago

Thanks for the reply. I thought it might still be required, e.g. when an extension uses require in a webworker?

But I think you're also right that the same issue applies to the amdX loader.

Before, this was always a defined value. But now that EcmaScript modules are used, this is undefined, causing an error when importing jschardet.

A possible workaround could be to manually overwrite the jschardet code: 

var $jscomp = { scope: {}, global: globalThis }, // change here
    Symbol;
$jscomp.initSymbol = function () {
    $jscomp.global.Symbol || (Symbol = $jscomp.Symbol);
    $jscomp.initSymbol = function () {};
};
jrieken commented 4 months ago

Thanks for the reply. I thought it might still be required, e.g. when an extension uses require in a webworker?

We fake require in that case. It's a single fetch and doesn't support dependencies ;-)

jrieken commented 4 months ago

It seems the issue is that this is undefined at the top level of Ecmascript modules:

@SimonSiefke The amdX module is actually a very simple AMD loader that we use to load our existing dependencies. This is a stepping stone so that the migration doesn't spread into all the places... When debugging the load-call, I see that this is defined and the error happens later when trying to access Symbol (which should also be defined...)

Image

I didn't get to the bottom of this yet, it is a really weird error. Things work when using the non-minified version which I'll do for now

SimonSiefke commented 4 months ago

@jrieken you're right. It looks like Symbol is defined but it's running in a vm Context where accessing Symbol.iterator is not allowed because it is a different v8 context (or something like that?).

It seems only the jschardet.min.js version has this extra Symbol code for supporting older browsers that don't support for .. of loops.

Tyriar commented 4 months ago

With https://github.com/xtermjs/xterm.js/pull/5092 xterm.js now ships an ESM module.

bpasero commented 4 months ago

Re jschardet: I opened https://github.com/aadsm/jschardet/pull/96 to try to get the project to update to a newer version of google-closure-compiler that they use for minification. I am not able to reproduce the issue anymore with newer versions. It is possible that maybe some polyfills (such as for...of) are not applied anymore when using the newer compiler versions.

For now I think its OK to use the non-minified release. Eventually we should consider to minify all our node module dependencies in our pipeline and not depend on how others do it.

Reported as https://github.com/aadsm/jschardet/issues/97

bpasero commented 3 months ago

After meeting with @jrieken some next steps and notes.

Active:

Up for Grabs:

Done:

bpasero commented 2 months ago

🚀 main is now capable of running as ESM, no more need for a branch!

Developing locally from main in ESM:

Tests in ESM:

bpasero commented 2 months ago

And we are happy for people willing to test our VS Code exploration builds that run with ESM. These builds can be side-by-side installed to VS Code and will update daily like insiders:

Feel free to report any issues you see, thanks!

bpasero commented 2 months ago

We have finished the majority of this work and this issue is closed with 2 follow up issues:

Thanks for being patient with us 🙏