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

importing the module doesn't work like shown in the readme for TypeScript projects #1932

Closed
1 task done
hood opened this issue Aug 14, 2021 · 26 comments
Closed
1 task done

Comments

@hood
Copy link

hood commented Aug 14, 2021

  • I've searched for any related issues and avoided creating a duplicate
    issue.

Description

The module must be imported with
import * as WebSocket from 'ws';
while the readme shows the module being imported with
import WebSocket, { WebSocketServer } from 'ws';

Reproducible in:

Any TypeScript project.

Steps to reproduce:

  1. install ws

  2. import it like import WebSocket, { WebSocketServer } from 'ws'; in a project

  3. try using the module as expected

Expected result:

The module can be imported as shown in the readme.

Actual result:

The module must be imported in alternative ways.

Attachments:

Just try running this piece of code

import WebSocket, { WebSocketServer } from 'ws';

const WSServer = new WebSocketServer();
@lpinca
Copy link
Member

lpinca commented Aug 14, 2021

$ cat index.mjs 
import WebSocket, { WebSocketServer } from 'ws';

console.log(WebSocket);

console.log(WebSocketServer);
$ node index.mjs 
[class WebSocket extends EventEmitter] {
  CONNECTING: 0,
  OPEN: 1,
  CLOSING: 2,
  CLOSED: 3
}
[class WebSocketServer extends EventEmitter]

It works fine as documented.

@saqirmdevx
Copy link

For me it does not work either.

Reproduction:
node --es-module-specifier-resolution=node ./build/app.js

Module '"ws"' has no exported member 'WebSocketServer'

TSConfig: 
    "target": "ES2019",
    "module": "ESNext", 

Code:

import WebSocket, { WebSocketServer } from 'ws';
....

export class SocketServer extends WebSocketServer {
    public static instance: SocketServer;

    constructor(server: Http.Server) {
        super(SocketServer._options(server));
        if (SocketServer.instance)
            return SocketServer.instance;

        SocketServer.instance = this;
    }

    private static _options(server: Http.Server): WebSocket.ServerOptions {
        return { clientTracking: false, noServer: true}
    }
}

This does not work, even client can not connect to the server via WebSocket

@hood
Copy link
Author

hood commented Aug 14, 2021

That is not what happens when you try and import ws in a project, unfortunately.

Just take a look here: https://replit.com/@javanotti/UnwieldyHotpinkRedundancy#index.ts

@lpinca
Copy link
Member

lpinca commented Aug 14, 2021

This is a TypeScript issue. Remove TypeScript and it works.

@hood
Copy link
Author

hood commented Aug 14, 2021

I maybe suboptimally worded the issue, but still this should be either adapted to work as expected or documented to show how to use the module in typescript. "Removing TypeScript" is not an option in TypeScript projects, I guess.

@hood hood changed the title importing the module doesn't work like shown in the readme importing the module doesn't work like shown in the readme for TypeScript projects Aug 14, 2021
@lpinca
Copy link
Member

lpinca commented Aug 14, 2021

We do not officially support TypeScript so it's up to TypeScript users to fix TypeScript issues.

The fix should be done in the TypeScript type definitions and not in ws documentation.

@lpinca
Copy link
Member

lpinca commented Aug 14, 2021

See https://github.com/DefinitelyTyped/DefinitelyTyped/blob/f7dc6fa52566fe3b/types/ws/index.d.ts. Those type definitions are for ws@7.

@lpinca
Copy link
Member

lpinca commented Aug 14, 2021

"Removing TypeScript" is not an option in TypeScript projects, I guess.

Yes, I understand but I think you can remove the @types/ws dependency until it is updated to support ws@8, or go back to ws@7 and use the old documentation.

@devanomaly
Copy link

Exploring source-code, it only seems that instead of {WebSocketServer} one needs to import {Server} (see index.d.ts, line 300)

@tamuratak
Copy link

@lpinca Is it intentional to export different names for the same class? I think it makes things unnecessarily complicated. I am not sure we can define @types/ws covering both cases.

WebSocketServer in wrapper.mjs

import WebSocketServer from './lib/websocket-server.js';

Server in index.js

ws/index.js

Line 6 in f38247e

WebSocket.Server = require('./lib/websocket-server');

@lpinca
Copy link
Member

lpinca commented Aug 15, 2021

WebSocketServer is the actual name of the class. It is exported as Server in CJS for historical reasons. Changing that would break a lot of code for no good reasons.

We could use Server in the ESM wrapper but

  1. It is a breaking change.
  2. I'm not very happy with it because I prefer to have the export name match the class name.

@lpinca
Copy link
Member

lpinca commented Aug 15, 2021

If there is no way to work around this in @types/ws, we could make WebSocket.WebSocketServer an alias for WebSocket.Server in index.js.

@tamuratak
Copy link

If there is no way to work around this in @types/ws, we could make WebSocket.WebSocketServer an alias for WebSocket.Server in index.js.

It would be very helpful.

@Pokute
Copy link

Pokute commented Aug 16, 2021

I suspect there is a problem with CJS and ESM versions having different interfaces due to the run-time shape being different.

import ws from 'ws';
ws.Server; // This doesn't exist.
ws.WebSocketServer; // This doesn't exist either.
const ws = require('ws');
ws.Server; // This exists

I'm not sure if typescript definitions support having different definitions for CJS and ESM.

@saqirmdevx
Copy link

I suspect there is a problem with CJS and ESM versions having different interfaces due to the run-time shape being different.

import ws from 'ws';
ws.Server; // This doesn't exist.
ws.WebSocketServer; // This doesn't exist either.
const ws = require('ws');
ws.Server; // This exists

I'm not sure if typescript definitions support having different definitions for CJS and ESM.

I have same issue, importing Default.Server returns undefined, importing WebSocketServer as individual works, but typescript is screaming with errors

lpinca added a commit that referenced this issue Aug 16, 2021
Add `WebSocket.WebSocketServer` as an alias for `WebSocket.Server` to
fix name consistency and improve interoperability with the ES module
wrapper.

Refs: #1932
@lpinca
Copy link
Member

lpinca commented Aug 16, 2021

Please take a look at #1935.

Anyway, FWIW, an ESM only declaration file as suggested by @Pokute in DefinitelyTyped/DefinitelyTyped#55151 (comment) makes sense to me now that ws uses an ES module wrapper. It does not seem bad to me to cut a new major version of @types/ws that only works with ws@>=8. This is why semver versioning exists.

That said, the Receiver and Sender classes should not be added in the declaration file as they are completely undocumented, most of their methods are private, and they should not be used directly. They are exported only to simplify method overriding in rare cases where it is needed.

@tamuratak
Copy link

DefinitelyTyped/DefinitelyTyped#55151 (comment) makes sense to me now that ws uses an ES module wrapper. It does not seem bad to me to cut a new major version of @types/ws that only works with ws@>=8. This is why semver versioning exists.

I think you had better have the .d.ts file in this project, not in @types.

I am the author of the PR. However, I am not one of the maintainers of the file. The decision of making breaking changes is up to them. I am not sure who is an active maintainer in the listed names in the file. With more people involved, the decision-making would be slow.

If you have the .d.ts file in this project, all you need is that you just decide.

@lpinca
Copy link
Member

lpinca commented Aug 17, 2021

@tamuratak I don't want to maintain the TypeScript type definitions. It's an additional burden and source of breaking changes I don't want to deal with. JSDoc annotations are sufficient for me.

@tamuratak
Copy link

Fair enough. I agree that it is a good practice for OSS maintainers to decrease the maintenance burden as much as possible. I have written your comment in the PR. I want to wait for the @types/ws maintainers' reply.

@saqirmdevx
Copy link

@tamuratak I think that Typescript support is necessary nowadays.

lpinca added a commit that referenced this issue Aug 17, 2021
Add `WebSocket.WebSocket` as an alias for `WebSocket` and
`WebSocket.WebSocketServer` as an alias for `WebSocket.Server` to fix
name consistency and improve interoperability with the ES module
wrapper.

Refs: #1877
Refs: #1932
lpinca added a commit that referenced this issue Aug 17, 2021
Add `WebSocket.WebSocket` as an alias for `WebSocket` and
`WebSocket.WebSocketServer` as an alias for `WebSocket.Server` to fix
name consistency and improve interoperability with the ES module
wrapper.

Refs: #1877
Refs: #1932
@lpinca
Copy link
Member

lpinca commented Aug 17, 2021

I've merged #1935 and I will cut a new release with it shortly. Once the WebSocket.WebSocketServer alias is added to @types/ws, the issue can be closed.

@Pokute
Copy link

Pokute commented Aug 17, 2021

I've merged #1935 and I will cut a new release with it shortly. Once the WebSocket.WebSocketServer alias is added to @types/ws, the issue can be closed.

Having working TypeScript definitions will still be impossible in my opinion, since WebSocket.WebSocketServer, WebSocket.Server and WebSocket.WebSocket are not defined for runtime ESM version. Please hold off a bit until releasing a new version until I can find a solution.

@lpinca
Copy link
Member

lpinca commented Aug 17, 2021

I think there is no viable solution to that. Things are different. What matters is that

import WebSocket, { WebSocketServer } from 'ws';

works as expected for TypeScript.

I think TypeScript will always use the CJS implementation as conditional exports are not supported.

@lpinca
Copy link
Member

lpinca commented Aug 17, 2021

With DefinitelyTyped/DefinitelyTyped#55151 (comment) and #1935, the above import statement should work fine.

If @types/ws is not used, then #1935 is sufficient.

$ cat index.ts
import WebSocket, { WebSocketServer } from 'ws';

console.log(WebSocket);
console.log(WebSocketServer);
$ tsc --version
Version 4.3.5
$ tsc --esModuleInterop index.ts
$ node index.js
<ref *1> [class WebSocket extends EventEmitter] {
  CONNECTING: 0,
  OPEN: 1,
  CLOSING: 2,
  CLOSED: 3,
  createWebSocketStream: [Function: createWebSocketStream],
  Server: [class WebSocketServer extends EventEmitter],
  Receiver: [class Receiver extends Writable],
  Sender: [class Sender],
  WebSocket: [Circular *1],
  WebSocketServer: [class WebSocketServer extends EventEmitter]
}
[class WebSocketServer extends EventEmitter]

@Pokute
Copy link

Pokute commented Aug 17, 2021

It still feels a bit over my head, but I read more about TS definition files and it seems like the runtime side might be all right. After that it's just some work to get the type definitions working properly.

@lpinca
Copy link
Member

lpinca commented Aug 18, 2021

I'm closing this as DefinitelyTyped/DefinitelyTyped#55151 should fix the issue once merged.

@lpinca lpinca closed this as completed Aug 18, 2021
th37rose added a commit to th37rose/websocket_server that referenced this issue May 24, 2022
Add `WebSocket.WebSocket` as an alias for `WebSocket` and
`WebSocket.WebSocketServer` as an alias for `WebSocket.Server` to fix
name consistency and improve interoperability with the ES module
wrapper.

Refs: websockets/ws#1877
Refs: websockets/ws#1932
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 a pull request may close this issue.

6 participants