Fix broken typings for consumers (5.0.0-next.1)

[tomrav]

I cloned vscode-css-languageservice locally to check 5.0.0-next.1 and noticed that the tests were breaking due to incorrect type definitions.
I removed the typings field from the package.json as TypeScript seemed to be able to find them regardless.

With this changed locally, all tests in vscode-css-languageservice pass successfully.

[dbaeumer]

The split is a breaking change and the rule we established is that the export on vscode-nls will only export the common types. If you need the node types as well you need to import vscode-nls\node.

See also the vscode-languageserver-node repository which follows the same style. It has an example of such an import https://github.com/microsoft/vscode-languageserver-node/blob/master/client/src/node/main.ts#L19

OK to close

[AviVahl]

Hold on @dbaeumer Having "typings" pointing to common.d.ts must be a mistake. It does not represent the shape of the package's public API, just the common code between the entrypoints.

This wasn't caught when I previously tested the new version, as I only did require('vscode-nls') (inside node REPL; to verify node evaluation) and import 'vscode-nls' (+bundle; to verify bundle-readiness). I've never checked the types.

Once I tried to:

import * as nls from 'vscode-nls';
const localize = nls.loadMessageBundle();

TypeScript will issue a type error saying loadMessageBundle is not available on nls. Surely not the behaviour we're aiming for.

Two possible resolutions:

• (this PR) Remove "typings" and let typescript resolve .d.ts based on which file it ended up resolving (using "main"). This works with the last several typescript versions. Older ones required you to always specify the "typings" field.
• Point "typings" to node.d.ts, as it contains the correct shape of the package.

[dbaeumer]

No, pointing to common is not a mistake. We decided that for VS Code code if you import the module you will only get the types that are useful between node and browser. If you want to have the node binding than you need to import vscode-nls/node. Otherwise it will be very easy to leak node types into browser code since dev would include vscode-nls.

For the loadMessageBundle. This needs to be fixed by having a declaration in common via an abstraction and having the concrete implementation in node / browser.

Please note that this doesn't influence the module loading in node and browser. These entries still point correctly to the node and browser implementation.

Are you willing to work on this.

[dbaeumer]

You might want to have a look at https://github.com/microsoft/vscode-languageserver-node/blob/master/jsonrpc/src/common/ral.ts#L1 to see how we do this in other VS Code npm modules.

[AviVahl]

The above is a very opinionated setup (install function; runtime check for every api call; duplicate type definitions with underscore prefix). I would rather not implement that myself.

If you want to have the node binding than you need to import vscode-nls/node.

This contradicts what the "browser" field is for. Its entire purpose is to have the entrypoint automatically re-mapped for you. Requiring consumers to explicitly specify an entrypoint defeats that and lowers the ability to write isomorphic code.

I can understand wanting a single place in the code that defines the shape of the public API (as an interface) and ensuring that it never imports Node specific apis (so they don't leak).

What I don't get is why would anybody want the proxy implementation with the stub methods. It adds overhead to to the speed, maintainability, and consumption of the library. It might seem "cute" when there are one or two public functions, but it fails to scale up well for wider APIs.

Since the changes you describe are internal to vscode-nls (ensuring it doesn't leak node types, etc), would you consider first merging this PR (which does fix the current state), and further refactor stuff in the future?

[dbaeumer]

If you look at the example I posted it requires a handful of abstractions for the whole LSP libraries running client side and server side to ensure that the common exposes a reasonable API. So it is actually not bad. I would say for vscode-nls it is a couple of hours to do that.

And there are two different things here: the runtime behavior and the compile time behavior. The runtime behavior is covered by the main and browser field, but the compile time behavior not. Unfortunately TS only supports one typings property for now (see for example microsoft/TypeScript#7753). Until this improves we decided to have a node and browser route to make it clear which API surface you want to use. This might go away in favor of a second property in the package.json file.

Sorry, I am against shipping a official version without having this clean.

[AviVahl]

Changing the entire way a library operates just to workaround a TypeScript limitation is not reasonable IMHO. Especially here, where the API surface is the same, and fallback behaviour is provided.

The test I added in the previous PR verifies that the exact same symbol names are exported at runtime. So it’s a matter of worrying about node types leaking due to node/main.d.ts being picked up. That file currently contains:

import { Options, LocalizeFunc, LoadFunc } from ‘../common/common’;
export { MessageFormat, BundleFormat, Options, LocalizeInfo, LocalizeFunc, LoadFunc, KeyInfo } from ‘../common/common’;
export declare function loadMessageBundle(file?: string): LocalizeFunc;
export declare function config(opts?: Options): LoadFunc;
//# sourceMappingURL=main.d.ts.map

There are no Node API references at all, as usages are internal to the lib. TypeScript doesn’t generate a /// <reference types=“node” /> in this case, and no actual leakage occurs.

The issue you are describing can happen (if someone references a Node-specific type in the public API), but is not actually happening right now. On the other end, the issue this PR solves happens right now in master and prevents us from running cleanly in the browser.

[tomrav]

As a side note to what Avi wrote above, this PR started as a single line contribution to try and alleviate a pain point for users like us. We are glad to improve and expand the fix to something more encompassing, but now it feels more like a chase after undocumented project compliance.

We’ve been trying to get this fix out for almost 2 months now and are doing our best to reply as promptly as we can when needed, please help us push this over the finish line.

[dbaeumer]

I can understand that you argue that way but I am pretty sure that you would not throw your guidelines at WIX over board simply because someone needs a feature.

We did what I described to the LSP libraries and we adopted the libraries in quite some LS to make them browser ready with great success and almost no effort on the client side.

I do see that is requires work to push this over the finish line but the work is better spent in the library once than later on with ever client since types start to leak.

Regarding our comments:

The main.d.ts will contain reference to node as soon as a node type will show up in the API. I agree that this is currently not the case but arguing that we should accept this will result in cases were it will leak since people will simply not know that it is forbitten to do so.

The current latest version of that library is still on 4.x which works without problems. This is why the new version got published as a .next version.

[dbaeumer]

I added a RAL support to the code (took 20 minutes 😃).

[AviVahl]

Alright, then this PR is now irrelevant. We'll wait for the next stable release.

I've looked at the RAL changes themselves... really not a fan. One additional side effect that this pattern has is not allowing tree-shaking to take place on the environment-specific code (it's always used when passed to install). In the future, if an esm target is also provided (with sideEffects: false), bundlers won't be able to eliminate the config method (for example) if unused. You may want to note that, as the pattern is used across several libraries.

[AviVahl]

@alexdima once master is released as stable, monaco-editor and monaco-editor-core will also benefit from the bundling compatibility. They currently use an inline mock for vscode-nls, which could be replaced with the official version.

[dbaeumer]

I think that depends on the bundler since the references to config / loadMessageBundle are internal. If it becomes an issue I can make the install two calls to help a bundler. If the bundler then still fails with this it will very likely fail with cyclic internal references in general.

[dbaeumer]

And actually the config could now go down in common since loadMessageBundle is there :-).

[dbaeumer]

Actually that gives the possibility to move more code into common and only leave the node specific code really sitting in the node/main

[AviVahl]

@dbaeumer any word on a stable release?

[dbaeumer]

Was out on vacation. Will look into it this week.

[dbaeumer]

Published [email protected] for a last check that packing is correct. If that works I can make 5.0.0

[AviVahl]

Tested on my end. Node evaluation, bundling compatibility, and type checking all appear to work.

[dbaeumer]

Great. Thanks for getting back.

[dbaeumer]

Will release a non next version beginning of next week.

[AviVahl]

heya @dbaeumer any update on this? we really need the bundle-compatible version.

[dbaeumer]

Sorry, too may thinks going on. Will raise priority of this :-)