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

Wrong extentions property type #1244

Closed
dcharbonnier opened this issue Nov 30, 2017 · 3 comments
Closed

Wrong extentions property type #1244

dcharbonnier opened this issue Nov 30, 2017 · 3 comments

Comments

@dcharbonnier
Copy link
Contributor

dcharbonnier commented Nov 30, 2017

The extension property of the client return an object, the websocket api expect a string.

this.extensions = {};

MDN documentation

Attribute Type Description
Extensions DOMString The extensions selected by the server. This is currently only the empty string or a list of extensions as negotiated by the connection.
@lpinca
Copy link
Member

lpinca commented Nov 30, 2017

We try to emulate the browser interface but in this case I don't see a reason to use a string.
We could use a private property for the extensions map and make extensions a getter.

get extensions () {
  return Object.keys(this._extensions).join(',');
}

but again I don't see any real advantage.

@dcharbonnier
Copy link
Contributor Author

I agree it's not so important for this property but in general I find it very interesting to have ws that implements Websocket as much as possible and it's already the case 👍
const ws: typeof WebSocket = typeof (WebSocket) !== "undefined" ? WebSocket : require("ws");

Making this a getter would make ws behave like the WebSocket specifications from what I know and I could remove one test here https://github.com/dcharbonnier/advanced-websocket/blob/develop/src/Waterfall.ts#L49

Was just to let you know, feel free to close it if you want.

@lpinca
Copy link
Member

lpinca commented Nov 30, 2017

I see, feel free to open a PR.

lpinca added a commit that referenced this issue Dec 26, 2017
Make `extensions` a getter that returns the negotiated extensions
names.

Fixes #1244
lpinca added a commit that referenced this issue Dec 27, 2017
Make `extensions` a getter that returns the negotiated extensions
names.

Fixes #1244
lpinca added a commit that referenced this issue Jan 4, 2018
Make `extensions` a getter that returns the negotiated extensions
names.

Fixes #1244
lpinca added a commit that referenced this issue Jan 4, 2018
Make `extensions` a getter that returns the negotiated extensions
names.

Fixes #1244
lpinca added a commit that referenced this issue Jan 5, 2018
Make `extensions` a getter that returns the negotiated extensions
names.

Fixes #1244
@lpinca lpinca closed this as completed in fdec524 Jan 5, 2018
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

No branches or pull requests

2 participants