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

VSCode client libraries have invalid exports #192144

Open
remcohaszing opened this issue Sep 4, 2023 · 3 comments
Open

VSCode client libraries have invalid exports #192144

remcohaszing opened this issue Sep 4, 2023 · 3 comments
Assignees
Labels
editor-core Editor basic functionality feature-request Request for new features or functionality
Milestone

Comments

@remcohaszing
Copy link
Contributor

remcohaszing commented Sep 4, 2023

VSCode has various client libraries that exist in separate repositories. Most of these packages have invalid package exports. As a result, it’s difficult to consume them in a package that is configured correctly. This means some packages can only be used if they are included via bundler hacks. Sometimes it’s the other way around, and they can be used with Node.js, but in a way they are no longer compatible with a bundler.

Currently this is blocking me from publishing a big MDX language server update (mdx-js/mdx-analyzer#340).

I want to help remedy this situation. I have already created some issues and pull requests, but perhaps it’s best to organize this from a central issue, hence this issue. Based on my experience working with language servers, Monaco Editor, and the VSCode list of useful Node.js modules, I have assembled the list below. A checkmark (✓) indicates everything is ok. A cross (❌) indicates there is a problem with a link to an explanation below. If there is no symbol, that indicates there’s no support for that module format, which is fine.

It also appears VSCode itself doesn’t depend on any of these modules. But some are generated from the VSCode repository. As far as I can tell, this means there is no need to support AMD nor UMD. Possibly relevant are #160416 and #166033.

Name CJS ESM Dependencies Notes / Related Issues
@vscode/emmet-helper ❌ Faux ESM
@vscode/extension-telemetry ❌ Browser Field
@vscode/l10n ❌ Browser Field ❌ Bundled Dependencies
@vscode/test-electron
jsonc-parser ❌ UMD ❌ Faux ESM microsoft/node-jsonc-parser#78
monaco-editor ❌ Faux ESM ❌ Bundled Dependencies Ships UMD, but it’s not exported.
monaco-editor-core ❌ Faux ESM ❌ Bundled Dependencies Ships UMD, but it’s not exported. Generated from the VSCode repository.
request-light ❌ UMD ❌ Browser Field ❌ Bundled Dependencies microsoft/node-request-light#24 microsoft/node-request-light#25
vscode-css-languageservice ❌ UMD ❌ Faux ESM
vscode-html-languageservice ❌ UMD ❌ Faux ESM
vscode-json-languageservice ❌ UMD ❌ Faux ESM microsoft/vscode-json-languageservice#200
vscode-jsonrpc ❌ Browser Field
vscode-languageclient ❌ Browser Field
vscode-languageserver ❌ Browser Export
vscode-languageserver-protocol ❌ Browser Field
vscode-languageserver-textdocument ❌ UMD
❌ Browser Implies ESM
vscode-languageserver-types ❌ UMD
❌ Browser Implies ESM
microsoft/vscode-languageserver-node#1297
vscode-markdown-languageservice
vscode-nls ❌ Browser Field microsoft/vscode-nls#52 Generated from the VSCode repository.
vscode-uri ❌ UMD
❌ Browser Implies ESM
❌ Bundled Dependencies microsoft/vscode-uri#38 Generated from the VSCode repository.

Faux ESM

Faux ESM means a package publishes ESM syntax, but it is not marked as such.

There are two ways to mark Node.js code as ESM:

  1. The ESM files use the .mjs extension, and .d.mts for file extensions.
  2. If package.json contains the following, .js and .d.ts files will be treated as ESM.
    {
      "type": "module"
    }

Dual published faux ESM packages often use the non-standard "module" field in package.json. Many module bundlers respect this field, causing imports to behave different when bundled from when using it with Node.js. Instead, packages should use package "exports" point to the correct location for CJS or ESM.

{
  "exports": {
    ".": {
      "import": "./path/to/esm/entrypoint.mjs",
      "default": "./path/to/cjs/entrypoint.cjs"
    }
  }
}

Also when using ESM, imports must include the file extension.

UMD

UMD is compatible with CJS. Node.js supports importing CJS as named imports, if the CJS is structured properly.

For example, the following CJS file:

module.exports.a = 'a'
exports.b = 'b'

Can be imported as such from ESM:

// 1)
import module from './commonjs.cjs'
// 2)
import { a, b } from './commonjs.cjs'

console.log({ a, b, module })

When using UMD, typically only 1) works, not 2). When dual publishing this with faux ESM, often only 2) works in bundlers, not 1).

Also this prevents tree shaking.

Bundled Dependencies

Some packages bundle their dependencies. When bundling dependencies, this means they are included in the package. If another package then depends on the bundled dependency, this means it gets duplicated, increasing the bundle size for the end user.

Browser Field

Some packages use the "browser" field in package.json. The intention is to tell bundlers to resolve a different module when targeting the browser. This is a non-standard field. The correct way to tell bundlers how to resolve imports for a browser, is using package "exports".

{
  "exports": {
    ".": {
      "browser": "./path/to/browser/entrypoint.js",
      "default": "./path/to/node/entrypoint.js"
    }
  }
}

Likewise, packages can define where packages are imported from conditionally using package "imports":

{
  "imports": {
    // Beware of https://github.com/nodejs/node/issues/49257 for CJS
    "#path": {
      "browser": "path-browserify",
      "default": "path"
    }
  }
}

ESM Implies Browser

Node.js can use ESM. Module bundlers can resolve imports to CJS, regardless of whether they target Node.js or browsers. Some packages incorrectly assume ESM is for the browser, and ESM is for Node.js. Instead, packages should use "exports" (See Browser Field).

Browser Implies ESM

Some packages resolve the browser condition to an ESM field. This means require() calls may lead to ESM, which is non-standard behaviour and may lead to unexpected error.

File Does Not Exist

This package points to a file that does not exist.

Default Export Compiled to CJS

Although I didn’t find any packages to which this apply, it’s good to add this to a list of pitfalls regarding CJS/ESM interop. When a default export is compiled to CJS, it becomes:

function someExport() {}

exports.default = someExport

This means that from native ESM, usage becomes:

import someExport from './commonjs.cjs'

someExport.default()

This is typically underable, so it’s best to avoid compiling default exports to CJS.

Dual Publishing

Various package dual publish ESM and CJS. Many people assume this has benefits, but they don’t truly understand the details. Really this mostly has downsides.

  • Dual publishing complicates the build process.
  • TypeScript wasn’t meant for dual publishing and becomes less helpful.
  • Because of ESM, you can’t use CJS APIs (require, module, __dirname, __filename).
  • Because of CJS, you can’t use ESM features (import statements, export statements, top-level await).
  • Because of CJS, you can’t use ESM-only dependencies.
  • You need to dual-publish TypeScript types too, and make sure they are correct.

Generally people are better off picking one module format. Choose CJS if you want your dependants to be abel to use CJS without using dynamic imports. Choose ESM if you want to help push the JavaScript ecosystem forward to a single module syntax.

@hediet
Copy link
Member

hediet commented Sep 12, 2023

Thanks for assembling this list!

What is your suggestion to avoid dual publishing?
Any change would probably be breaking here.

@zm-cttae
Copy link

zm-cttae commented Sep 14, 2023

Recently an update to unofficial lib vscode-textmate-languageservice added some APIs highly requested in core:

Could you assess the library for its export validity?

@remcohaszing
Copy link
Contributor Author

Avoiding dual publishing means to publish either CJS or ESM, not both.

CJS

To publish CJS only, requires the following setup:

// package.json
{
  // This is default
  // "type": "commonjs",

  // One of these should be specified. "main" points to the package entry point. "exports" points to the package entry point and makes everything else private.
  // "main": "./dist/index.js",
  // "exports": "./dist/index.js",

  // This is only needed to support the legacy "node10" module resolution, formerly known as "node", if "exports" is used.
  // I would omit it, as no one should be using that anymore
  // "types": "./dist/index.d.ts"

  "scripts": {
    "prepack": "tsc -b"
  }
}
// tsconfig.json
{
  "compilerOptions": {
    "module": "nodenext",
    // Ideally one of the following. I like the latter, I think the TypeScript team recommends the former.
    // "verbatimModuleSyntax": true,
    // "esModuleInterop": false,
  }
}

When not writing default exports, this works exactly as expected in from both CJS and ESM dependants. The downside is, this means we can’t use ESM dependencies.

This removes support from AMD/UMD. This is technically breaking, but personally I don’t think anyone is affected by this. In fact, this solves an annoying warning that some build tools show:

Next.js warning

ESM

To publish ESM only, requires the following setup:

// package.json
{
  "type": "module",

  // One of these should be specified. "main" points to the package entry point. "exports" points to the package entry point and makes everything else private.
  // "main": "./dist/index.js",
  // "exports": "./dist/index.js",

  // This is only needed to support the legacy "node10" module resolution, formerly known as "node", if "exports" is used.
  // I would omit it, as no one should be using that anymore
  // "types": "./dist/index.d.ts"

  "scripts": {
    "prepack": "tsc -b"
  }
}
// tsconfig.json
{
  "compilerOptions": {
    "module": "nodenext",
  }
}

The biggest upside is that you can use ESM dependencies. Also the existence of two module formats in the JavaScript JavaScript ecosystem is hard to deal with for many users. Personally I believe ESM is the future and it’s best to bite the bullet and go ESM only.

This means users of the package also need to go ESM-only.

Currently VSCode extensions don’t support ESM. But I don’t think this is a problem. As far as I know VSCode extensions should bundle their dependencies, so the module format doesn’t really matter.

Dual Publishing

Despite the quirks of dual publishing, it is possible to do so. This means using "main" is not an option, so we would have to use "exports", marking all other package files as private. This could technically be considered breaking.

// package.json
{
  "exports": {
    "import": "./dist/esm/index.js",
    "default": "./dist/cjs/index.js"
  },

  // This is only needed to support the legacy "node10" module resolution, formerly known as "node", if "exports" is used.
  // I would omit it, as no one should be using that anymore
  // "types": "./dist/index.d.ts"

  "scripts": {
    // The fix-esm script writes a `package.json` file as in https://github.com/microsoft/vscode-languageserver-node/pull/1297/files#diff-4ac8cc0c9b1dd409f838c35220acc380491c4aef20dfba60844c576f1354158a
    "prepack": "tsc -b && node ./scripts/fix-esm"
  }
}

So, some breaking changes are:

  • Hacks for resolving umd to esm could break. This is because they are no longer necessary.
  • Using exports breaks imports of internals. The only project of which I’m aware it depends on this, is yaml-language-server for vscode-json-languageservice. Currently it has an exact version pinned, so it won’t just break. To support their use case, it would be best to export the things they need from vscode-json-languageservice as part of the public API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor-core Editor basic functionality feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

4 participants