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

ESM support for VSCode and extensions #212727

Draft
wants to merge 773 commits into
base: main
Choose a base branch
from

Conversation

SimonSiefke
Copy link
Contributor

@SimonSiefke SimonSiefke commented May 14, 2024

This PR adds ESM support for VSCode and extensions. Fixes #160416. Fixes #130367

Try Out

git clone git@github.com:SimonSiefke/vscode.git &&
cd vscode &&
git checkout simon/esm &&
yarn &&
yarn compile &&
./scripts/code.sh

A sample ESM extension is located at extensions/hello-esm. Running the command hello esm displays a hello world notification:

esm-extension

Feedback / Next Steps

It is probably too large to review and would be good to split up into smaller PRs! Before continuing it seems it would be good to gather some feedback from the VSCode team!


Changes

Type Imports

Some imports are changed from import { IDisposable } from 'vs/base/common/lifecycle'; to import type { IDisposable } from 'vs/base/common/lifecycle'; to work with ESM.

Assert imports

Most of the changes in this PR are due to the assert import. In ESM, when using import * as assert from 'assert' , assert cannot be a function. The change is import assert from 'assert'

Xterm imports

import { Terminal } from '@xterm/headless'

is changed to

import xtermHeadless from '@xterm/headless';

const { Terminal } = xtermHeadless;

This makes the import work in ESM. It seems it could even be changed back when @xterm/headless is published as ESM.

Css Imports

import 'vs/css!./actionbar' is compiled into importCss('./actionbar.css', import.meta.url). The importCss function uses document.createElement('link') to create a stylesheet for actionbar.css.
`

AmdX Loader

The amdx loader is changed from loading scripts with document.createElement('script'); or importScripts to load scripts using import.

ESM extensions

To support ESM extensions, the extension host is started with a custom loader NODE_OPTIONS: --import="vscode/src/extension-loader-register.js"`.

The loader can change how ESM files are imported, so that import vscode from "vscode" works as expected:

// extension-loader.js
export async function resolve(specifier, context, nextResolve) {
	if (specifier === "vscode") {
		return {
			shortCircuit: true,
			format: "module",
			url: new URL("./fake-vscode.js", import.meta.url).toString(),
		};
	}
	return nextResolve(specifier, context);
}

export function load(url, context, nextLoad) {
	return nextLoad(url);
}

When importing vscode, this fake-vscode.js file is imported instead:

// fake-vscode.js
const api = globalThis.vscodeFakeApi
if (!api) {
	throw new Error('vscode api not available')
}

const { window, commands } = api

export { window, commands }

vscodeFakeApi is created by extensionApiFactory() and provides the vscode api properties window, commands and more.

NodeJS Docs for module customization hooks: https://nodejs.org/api/module.html#customization-hooks

Tests

To support ESM for electron integration tests, import maps and preload require are used.

<script type="importmap">
	{
		"imports": {
			"fs": "./import-map/fs.js"
		}
	}
</script>
// preload.js
window.testGlobalRequire = require // expose require function for tests
// import-map/fs.js

const fs = window.testGlobalRequire('testGlobalRequire')

const { createWriteStream, existsSync, readFileSync, readdirSync, rmSync, writeFileSync, unwatchFile, watchFile, watch, readFile, mkdir, chmod, link, unlink, truncate, copyFile, read, access, open, rm, readdir, writeFile, constants, stat, mkdirSync, unlinkSync, fdatasync, statSync, accessSync, symlinkSync, openSync, createReadStream, realpathSync, lstatSync, promises, lstat, fdatasyncSync, closeSync, utimes, appendFile, close, symlink, readlink, rmdir, write, renameSync, rename, realpath } = fs;

export { createWriteStream, existsSync, readFileSync, readdirSync, rmSync, writeFileSync, unwatchFile, watchFile, watch, readFile, mkdir, chmod, link, unlink, truncate, copyFile, read, access, open, rm, readdir, writeFile, constants, stat, mkdirSync, unlinkSync, fdatasync, statSync, accessSync, symlinkSync, openSync, createReadStream, realpathSync, lstatSync, promises, lstat, fdatasyncSync, closeSync, utimes, appendFile, close, symlink, readlink, rmdir, write, renameSync, rename, realpath };

It's a lot of changes for every imported module (assert, child_process, cookie, crypto, electron, events, fs, graceful-fs, istanbul-lib-coverage, istanbul-lib-instrument and more) but it makes the tests work with ESM.

Other

No ESM support for web extensions yet

Making use of NodeJS loader hooks, the ESM support only applies to NodeJS extensions. There would be no ESM support for web extensions yet.

@jrieken
Copy link
Member

jrieken commented May 15, 2024

Thanks for this huge effort!

Haven't had the chance to drill into code-details yet but based on your description it seems that you have found the right places and are making good decisions. In the alex/esm branch we do some things similar and some different. You have also covered extensions (which we haven't) and that's something I would like to learn from you.

How to continue is a very good question. Reviewing such a huge, cross-cutting PR is a big challenge for us. Also #166033 exists. It hasn't gotten much attention but our effort is not abandoned. For instance, this milestone we are looking into decoupling NLS from module-loading (#212542). That's one of the big remaining items of the todo-list and also outlines our strategy: whenever possible we try to make changes in main so that the ESM PR stays small and has less chance of conflict. E.g that's why amdX exists - it is an (incomplete) AMD-loader that can be used for things like xterm that aren't ESM yet.

So, that would be my general recommendation (sorry, without having looked into details yet):

  • let's try to find things that can be done in main
  • let's find things that can be done in alex/esm

I don't wanna make false promises but I have the slight hope that the NLS-work and me finishing some AI-APIs frees me up next month to get back into this. From the top of my head, there is left

  • merge main 🙈
  • electron now supports ESM and we can remove/adjust our workaround
  • extensions: nodejs and web with real API instances. My hope and gut-feeling is that imports maps can be helpful for this.
  • (... things I will remember once back into it...)

Thanks and Happy Coding

@SimonSiefke
Copy link
Contributor Author

@jrieken Thanks for the great feedback!

I very much agree it would be useful to merge smaller/more focused ESM related PR's into main.

One small change towards ESM could be typescript isolatedModules: More details in #212835

I am also looking forward to the NLS work! Thanks!

@jrieken
Copy link
Member

jrieken commented May 16, 2024

One small change towards ESM could be typescript isolatedModules: More details in #212835

We are heavy users and fans on const enum which makes isolatedModules a real hard sell.

I have been thinking about esModuleInterop. I believe that's safe to do in main and will allow to make the assert-refactoring, even case-by-case to prevent monster PRs

@SimonSiefke
Copy link
Contributor Author

SimonSiefke commented May 16, 2024

We are heavy users and fans on const enum which makes isolatedModules a real hard sell.

I think const enums can still be used with typescript isolated modules, one would not need to change all const enums to enums. :)

I have been thinking about esModuleInterop. I believe that's safe to do in main and will allow to make the assert-refactoring, even case-by-case to prevent monster PRs

Yes, I think that would be great, also!

@jrieken
Copy link
Member

jrieken commented May 16, 2024

Haven't tried it but this sounds like they would raise a compile error: https://www.typescriptlang.org/tsconfig/#references-to-const-enum-members?

@SimonSiefke
Copy link
Contributor Author

SimonSiefke commented May 16, 2024

I finished the all changes now for isolated modules, 7 files changed: #212913

I'm looking forward to feedback! :)

@SimonSiefke
Copy link
Contributor Author

And a Pull Request for esModuleInterop: #213014

@SimonSiefke
Copy link
Contributor Author

SimonSiefke commented May 27, 2024

Some more ideas what (perhaps) could be done in the main branch:

Using script type module

Using <script type="module"> would allow using imports. Even though imports are not used now, it seems it would be quite useful for later. #213689

Using worker type module

Similar to above, this would enable using import in workers. Probably some kind of workaround may be needed to keep importScripts working, which is not available in module workers.

Moving src/buildfile.js into the build folder

With the src files being ESM, I encountered some issues with the src/buildfile.js (Error [ERR_REQUIRE_ESM]: require() of ES Module '../src/buildfile.js' not supported). It seems to me it might be useful to move it to the build folder, avoiding the ERR_REQUIRE_ESM error.

AMD Loader error with ESM

When using the AMD loader with ESM, this at the top level of an ESM module is undefined. This causes an error with the AMD loader. microsoft/vscode-loader#58



Feedback and ideas are as always very welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore AMD to ESM migration Enable consuming of ES modules in extensions
2 participants