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

Align all tsconfigs and vscode-ws-jsonrpc provides esm/cjs #390

Merged
merged 2 commits into from
Jun 22, 2022

Conversation

kaisalmen
Copy link
Collaborator

Added typesVersions to monaco-languageclient and vscode-ws-jsonrpc, so exports can be properly imported with TS:

I did not understand why the "exports" lead to import problems in typescript (node example) until I found this:
https://github.com/teppeis/typescript-subpath-exports-workaround

vscode-ws-jsonrpc gets a 1.0.1 release once this is merged.
and it makes sense to release a 2.0.2 of monaco-languageclient, because of the typesVersions. Do you have other things that should be fixed as well?

- Added typesVersions to monaco-languageclient and vscode-ws-jsonrpc, so exports can be properly imported with TS
## BREAKING CHANGES

* If you use Webpack or vite for bundling, you have to remove the `vscode` alias entry from the configuration
* If you customized monaco services, then you have to add them
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by then you have to add them? The interfaces just changed

Copy link
Collaborator Author

@kaisalmen kaisalmen Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"adjust them" or "adjust them to the changed interfaces" instead of "add them". Don't know why I wrote this!? 😊

Do we have an example for that inside this repo?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so... I think the most important points is the Workspace taking the uri and workspaceFolders instead of a path, and the monaco namespace not required to be provided

@CGNonofr
Copy link
Collaborator

CGNonofr commented Jun 22, 2022

Do you have other things that should be fixed as well?

I admit I didn't experience any issue with exports

@@ -7,7 +7,7 @@ import * as http from 'http';
import * as url from 'url';
import * as net from 'net';
import express from 'express';
import * as rpc from 'vscode-ws-jsonrpc';
import * as rpc from 'vscode-ws-jsonrpc/cjs';
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CGNonofr this did not work before I added the typedVersions. tsc/vscode complain it is not defined

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to use the cjs version here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought. This is a node application and when I switched to modules/esm then I ran into import errors beyond vscode-ws-jsonrpc. Before everything was commonjs and it worked. I ensure it works the same way now, too. Here no bundler is involved (vite or webpack could handle this problem)

I need to look into module only node/ts, but I just wanted to make it work again for now and provide people with options.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I use node --loader ts-node/esm instead of node -r ts-node/register in my projects, maybe it helps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can give it a try

@kaisalmen
Copy link
Collaborator Author

Shall I remove the AMD and CJS build from monaco-languageclient with this PR as well. We only keep monaco-converter and monaco-converter/cjs for BW-compatibility reasons, then?

@CGNonofr
Copy link
Collaborator

Shall I remove the AMD and CJS build from monaco-languageclient with this PR as well. We only keep monaco-converter and monaco-converter/cjs for BW-compatibility reasons, then?

Yep! I wonder why anything else than ESM would be useful in 2022. And then I noticed vscode-languageclient is still in CJS only 😞

@kaisalmen kaisalmen changed the title Align all tsconfigs and vscode-ws-jsonrpc provides sem/cjs Align all tsconfigs and vscode-ws-jsonrpc provides esm/cjs Jun 22, 2022
@kaisalmen
Copy link
Collaborator Author

@CGNonofr I update the CHANGELOGs (sorry for the dash changes).

Do you think the BREAKING CHANGE section is now better (in both packages)?

@CGNonofr
Copy link
Collaborator

@CGNonofr I update the CHANGELOGs (sorry for the dash changes).

Do you think the BREAKING CHANGE section is now better (in both packages)?

Looks good 👍

@kaisalmen
Copy link
Collaborator Author

Ready for merge then?

@kaisalmen kaisalmen merged commit 756c30f into main Jun 22, 2022
@kaisalmen kaisalmen deleted the align-tsconfigs branch June 22, 2022 14:35
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.

None yet

2 participants