fix: ensure bundlers pick up esm version

[AviVahl]

up until the addition of "exports", bundlers found the root "module" field and resolved the the esm version.

this change fixes a regression from #1297 where cjs browser consumers now get resolved to the umd, getting a warning about dynamic require.

[dbaeumer]

@aeschli Martin, can you please have a look.

[AviVahl]

This is easier to review if u ignore the whitespace changes: https://github.com/microsoft/vscode-languageserver-node/pull/1326/files?diff=unified&w=1

[aeschli]

The change makes sense, but please revert the indentation changes...

[AviVahl]

done. 👍

[remcohaszing]

IMO it’s a good thing there is no distinction between usage in the browser and other platforms. This could open up another set of issues.

I recommend against merging this. However, I do recommend to change the UMD bundle to a regular CJS bundle. That will also make the warning go away. I suggest to merge #1328 instead.

[AviVahl]

Either way works for us. If it won't point to a umd, the warning would go away.

That being said, esm allows for better tree shaking, which might be preferable for browser builds.

[aeschli]

As we are not quite ready yet to abandon AMD, I would go with the proposed fix to set browser to the ESM bits.

[remcohaszing]

This change basically says: If the browser condition is set, require('vscode-languageserver-types') call should resolve to an ESM file. So for example:

node --conditions browser --test

// test.js
// This is an error when the browser condition is used
const types = require('vscode-languageserver-types')

This is just one example. I’m not sure how different bundlers and test runners deal with this.

[AviVahl]

This change basically says: If the browser condition is set, require('vscode-languageserver-types') call should resolve to an ESM file. So for example:

node --conditions browser --test

// test.js
// This is an error when the browser condition is used
const types = require('vscode-languageserver-types')

This is just one example. I’m not sure how different bundlers and test runners deal with this.

Both webpack and esbuild set this condition on their resolvers when targeting browser bundles. My team already saw several npm packages using this out there in the wild.

Not being able to run in node with --conditions browser set seems fine to me. I mean... it would also break if I use --conditions types, as these are used to point to .d.ts files.

We use these conditions to address special cases. Forcing those custom conditions in cases that work OOTB (e.g. node as-is) doesn't seem logical to me.

If this package was built to both esm and cjs, this "browser" condition could have had nested conditions under it, providing import/require targets which are bundle-compatible (without any dynamic require usage). That wouldn't necessarily mean we would be able to use --conditions browser in node, as many other npm packages don't provide both targets under the "browser" condition.

[remcohaszing]

Both webpack and esbuild set this condition on their resolvers when targeting browser bundles. My team already saw several npm packages using this out there in the wild.

I think it’s very useful to be able to do this. This allows to provide different implementations when targeting a browser or Node.js. For example, the browser entry point may be able to use the DOM, or the Node.js might use Node-only APIs. I don’t think it’s a good idea to use this to force require to resolve resolve to ESM files.

Not being able to run in node with --conditions browser set seems fine to me. I mean... it would also break if I use --conditions types, as these are used to point to .d.ts files.

We use these conditions to address special cases. Forcing those custom conditions in cases that work OOTB (e.g. node as-is) doesn't seem logical to me.

The browser condition and the types condition are not the same thing. The node --conditions types deliberately breaks the program. Running node --conditions browser has an actual use: To test browser code using Node.js, i.e. leveraring jsdom and the Node.js test runner.

If this package was built to both esm and cjs, this "browser" condition could have had nested conditions under it, providing import/require targets which are bundle-compatible (without any dynamic require usage). That wouldn't necessarily mean we would be able to use --conditions browser in node, as many other npm packages don't provide both targets under the "browser" condition.

Yes, it would be possible to triple-publish ESM for ESM supporting targets, CJS for bundlers, and UMD for Node.js. I don’t think anyone would want to go into that direction though.

[AviVahl]

I'm not sure your suggestion is then. The PR you've referenced to is a much wider change.
Having the cjs resolved instead of a UMD by default could be a breaking change. No-UMD build is also a no-go, as some of the vscode ecosystem still needs the AMD version. There's always the possibility of exposing those extra umds as package subpaths, or a custom "umd"/"amd" conditions, but that would require additional consumer changes.

While I can imagine a reality in which people execute --conditions browser to test with jsdom, this would require all participating packages to support the two targets that node might end up wanting (cjs or esm, depending on the consumer). Currently most packages we saw simply point to the esm version.

The UMD causes bundling warnings, and is pointed to by the new "exports" definition which takes precedence over the previous root fields ("main"/"module"/"browser").
Previous version of the vscode-languageserver-types package pointed bundlers to the esm version via the "module" field, avoiding this issue.

Possible backward-compatible solutions I see are:

1. (this PR)

{
	"exports": {
		".": {
			"browser": "./lib/esm/main.js",
			"import": "./lib/esm/main.js",
			"default": "./lib/umd/main.js"
		}
	},
}

2. introducing a new commonjs target and doing:

{
	"exports": {
		".": {
			"browser": {
				"import": "./lib/esm/main.js",
				"require": "./lib/cjs/main.js"
			},
			"import": "./lib/esm/main.js",
			"default": "./lib/umd/main.js"
		}
	},
}

The second one seems like an overkill, but would address any requirement to execute node --conditions browser, if such exists. One could add browser->default as well, but lets leave that outside the scope.

[remcohaszing]

Note that the current behaviour isn’t broken. The bundler correctly resolves from CJS to the CJS file, which happens to be a UMD file. The bundler now shows a warning, not an error.

How are you pulling in the vscode-languageserver-types dependency? Most of the VSCode client libraries have related issues, but solving it starts at the most low-level packages such as this one. microsoft/vscode#192144

[AviVahl]

We're directly importing it in our code, using it for integrations with monaco and vscode.

So your suggestion is to keep the new warning? In our case, it is the only warning during build of over 500 deps.

I find that is very odd, just to allow running node --conditions browser, which is not really used by anyone nor this project.

I think this should land as-is. The argument for not doing so is not compelling enough IMO.

[remcohaszing]

This means you are consuming the imports from a faux ESM context. The solution is to switch to real ESM. Bundlers treat this pretty well nowadays, which is exactly the reason it’s resolving to the CJS entrypoint for you.

If you wish to use ESM, you should either

1. specify "type": "module" in your package.json; or
2. rename your .js files to .mjs and your .ts files to .mts.

Note that using a bundler doesn’t mean the same as targeting a browser. Your use case is a good example of this. You’re bundling a VSCode plugin, which I imagine targets Node.js, not the browser.

[AviVahl]

The reality is that we have a commonjs consumer that gets bundled. Changing it to native esm isn't easy. We're talking about huge projects with numerous integrations. We're talking node, electron (renderer and main), web worker, worker thread, you name it.

I mean, we're working on it, but both TypeScript and node leave a lot to be desired when migrating large projects.

Using "type": "module", for example, causes typescript to misbehave in commonjs consumers with moduleResolution: "node16"/"bundler" set. It warns about hybrid packages being esm-only, even though they have a "require" condition for their main entrypoint.

EDIT: not to mention Node and their lexer for detecting named imports from commonjs. Not to mention node ignoring __esModule flag for commonjs that originated from esm, and how it messes default export/import in that case. TypeScript is "fun" around those places as well. See microsoft/TypeScript#55221

My point being: I'm really aware, and I still think this fix is fine given the current state of the ecosystem.

[aeschli]

We go with

{
	"exports": {
		".": {
			"browser": "./lib/esm/main.js",
			"import": "./lib/esm/main.js",
			"default": "./lib/umd/main.js"
		}
	},
}

for now which is equivalent to what we had before when w had the module and browser entry points.
If node --conditions browser becomes popular and we get more requests, we can still consider looking at 2.

[remcohaszing]

It sounds like what the OP is really looking for, is to always resolve to ESM, even from CJS. If you want this, the bundler should be configured to use the import condition, not the browser condition. This isn’t just about vscode-languageserver-types, it’s about all of their dual-published dependencies.

Using "type": "module", for example, causes typescript to misbehave in commonjs consumers with moduleResolution: "node16"/"bundler" set. It warns about hybrid packages being esm-only, even though they have a "require" condition for their main entrypoint.

TypeScript is really correct about this nowadays. If it warns about hybrid packages being esm-only, that means those packages are misconfigured. I know there are still a lot of misconfigured packages out there, and I’m trying to fix as much as possible all around the ecosystem. That’s one reason why I fixed it for vscode-languageserver-types and why I oppose the changes in this PR.

[aeschli]

@remcohaszing Thanks so much for your input, Very happy to have your expertise (even if I ignored it this time). To quickly get unblocked we're going with (1.) for now as the existing build scripts for the web extensions that are currently broken.

Would it have made sense to keep the browser entry point?

[remcohaszing]

I understand you need to make some pragmatic choices. I fear this change will cause more issues, because it’s technically incorrect. I hope it won’t be as bad, I find this hard to predict.

Dual packaging comes with a lot of quirks. I think a huge amount of issues would already be solved if we can get rid of all UMD bundles.

I think the browser field is almost as bad as the browser export. Although browser usage pretty much implies a bundler is used, bundling doesn’t imply the code is built for the browser.