Changed browser to es modules #20
Conversation
| @@ -14,4 +14,4 @@ if (typeof WebSocket !== 'undefined') { | |||
| ws = self.WebSocket || self.MozWebSocket | |||
| } | |||
|
|
|||
| module.exports = ws | |||
| export default ws | |||
There was a problem hiding this comment.
It looks like a breaking change, the major version number should be changed when we publish it🤔
| } else if (typeof self !== 'undefined') { | ||
| ws = self.WebSocket || self.MozWebSocket; | ||
| } | ||
|
|
There was a problem hiding this comment.
should we throw an error when no WebSocket detected, like ws did: https://github.com/websockets/ws/blob/master/browser.js
There was a problem hiding this comment.
can better use "globalThis"?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis
especially since most modern browsers work from a regular WebSocket
https://caniuse.com/mdn-api_websocket
| format: "es", | ||
| file: "app.output.js", | ||
| }, | ||
| plugins: [resolve()], |
There was a problem hiding this comment.
I tried this config and got an error, does here lost { browser: true } option?
There was a problem hiding this comment.
Yes, resolve() needs option browser: true I believe to replicate behaviour.
| } else if (typeof global !== 'undefined') { | ||
| ws = global.WebSocket || global.MozWebSocket; |
There was a problem hiding this comment.
Moreover, checking for global is useless here, because this is a node.js object
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis#description
Historically, accessing the global object has required different syntax in different JavaScript environments. On the web you can use window, self, or frames - but in Web Workers only self will work. In Node.js none of these work, and you must instead use global.
|
Wouldn't it be better to just do it like this? export default WebSocketlol 😅 |
|
@heineiuo @guillemcordoba Are there any plans to further advance this issue? |
|
When will this request be merged? @heineiuo have the errors been resolved? |
|
As many people are waiting this change to be landed, so I think a good solution is just merge this PR and open a new PR to solve the errors in examples. |
|
Released a beta version ( |
@heineiuo I changed the
browser.jsto be es modules, which is what all bundlers and browsers actually understand.I could do this in a separate file, but then I think it would be quite tricky to separate when to import esmodules vs commonjs ones, when the "module"/"main" keywords are used to distinguish the node and browser environment.
Also added rollup example.