Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explore AMD to ESM migration #160416

Closed
Tracked by #3
jrieken opened this issue Sep 8, 2022 · 31 comments
Closed
Tracked by #3

Explore AMD to ESM migration #160416

jrieken opened this issue Sep 8, 2022 · 31 comments
Assignees
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc. ESM Issues caused by our AMD => ESM work on-testplan plan-item VS Code - planned item for upcoming
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Sep 8, 2022

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:

  • we have a special AMD loader plugin that supports nls
  • we also have a special plugin for CSS
  • we have special bundle logic for both NLS and CSS dependencies
  • we use require.toUrl to locate sibling resources, like images, this needs to migrate onto import.meta.url

Backlog Plan: #160416 (comment)

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
@jrieken jrieken self-assigned this Sep 8, 2022
@jrieken jrieken added debt Code quality issues engineering VS Code - Build / issue tracking / etc. labels Sep 8, 2022
@jrieken jrieken added this to the September 2022 milestone Sep 8, 2022
@jrieken jrieken modified the milestones: September 2022, October 2022 Sep 28, 2022
@lorand-horvath

This comment was marked as resolved.

@jasonwilliams
Copy link
Contributor

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
Copy link
Member Author

jrieken commented Jan 23, 2023

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
Copy link
Contributor

@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
Copy link
Member Author

jrieken commented Jan 23, 2023

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

@zm-cttae
Copy link

zm-cttae commented Feb 14, 2023

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/

@jrieken jrieken modified the milestones: February 2023, March 2023 Feb 20, 2023
@SimonSiefke
Copy link
Contributor

@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 microsoft/vscode-loader#58 that might fix it.

@bpasero
Copy link
Member

bpasero commented Jul 5, 2024

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

@SimonSiefke
Copy link
Contributor

SimonSiefke commented Jul 5, 2024

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
Copy link
Member Author

jrieken commented Jul 5, 2024

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
Copy link
Member Author

jrieken commented Jul 5, 2024

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
Copy link
Contributor

@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.

@joaomoreno joaomoreno added the plan-item VS Code - planned item for upcoming label Jul 6, 2024
@joaomoreno joaomoreno modified the milestones: On Deck, July 2024 Jul 6, 2024
@Tyriar
Copy link
Member

Tyriar commented Jul 7, 2024

With xtermjs/xterm.js#5092 xterm.js now ships an ESM module.

@bpasero
Copy link
Member

bpasero commented Jul 9, 2024

Re jschardet: I opened aadsm/jschardet#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 aadsm/jschardet#97

@bpasero
Copy link
Member

bpasero commented Aug 5, 2024

After meeting with @jrieken some next steps and notes.

Active:

  • 🏃 Prepare for merging esm transformations to main while being able to build old amd variant @bpasero
  • 🏃 @vscode/tree-sitter-wasm fails to import via importAMDNodeModule with Cannot resolve dependencies...The dependencies are: exports @jrieken

Up for Grabs:

  • All TODO@esm entries

  • today our workbench-dev.html cannot use type="module" for script tags because of import maps usage

Done:

  • monaco ESM build fails (refs) @bpasero
  • Selfhost: Coloring can break with TypeError: Cannot set properties of undefined (setting 'vscodetextmate') @bpasero
  • Fails to load dependency @microsoft/1ds-core-js due to dependencies in the module, try to load as ESM? @bpasero
  • cannot run unit tests anymore (scripts/test.sh)
    • "TypeError: Failed to fetch dynamically imported module: file:///Users/bpasero/Development/Microsoft/vscode-esm/out/vs/platform/extensionManagement/test/node/extensionDownloader.test.js"
  • there are still some require calls in our sources in welcome (here, here)
  • editorWalkThrough imports a markdown document in a strange way breaking ESM (refs)
  • ASAR support is currently disabled and would need rework for ESM if we want it (refs)
    • we will disable ASAR in ESM and work on a different solution via esbuild to inline our dependencies
  • All: REH+Web does not start with unauthorized error (vsda related?) @bpasero @jrieken
  • there is a hardcoded entry point to vs/base/worker/workerMain because we do not support the dest property in buildfile.js yet (refs, refs) @jrieken
  • removeDuplicateTSBoilerplate is disabled and needs to be adopted for esbuild: boilerplate ends up being named with increasing numbers (e.g. decorate200) due to the build making everything flat and our code would need to update the references somehow. @jrieken Trying to tackle this with the importHelpers-flag and tslib
  • the fileContentMapper option for AMD task is not yet supported in the ESM world (refs) @jrieken
  • process explorer loads with broken CSS-
  • Linux build is unhealthy, fails with RPM packaging (refs)
  • macOS universal bundling fails

@bpasero bpasero added the ESM Issues caused by our AMD => ESM work label Aug 12, 2024
@bpasero
Copy link
Member

bpasero commented Aug 15, 2024

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

Developing locally from main in ESM:

  • node migrate.mjs (this will copy src to src2 with ESM transformations)
  • yarn watch-esm (this will compile from src2 to out)
  • scripts/code.sh

Tests in ESM:

  • scripts/test-esm.sh
  • yarn test-browser-esm
  • yarn test-node-esm
  • scripts/test-integration-esm.sh

@bpasero
Copy link
Member

bpasero commented Aug 16, 2024

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
Copy link
Member

bpasero commented Aug 23, 2024

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

Thanks for being patient with us 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc. ESM Issues caused by our AMD => ESM work on-testplan plan-item VS Code - planned item for upcoming
Projects
None yet
Development

Successfully merging a pull request may close this issue.