y-js / y-websockets-client

Websocket connector for Yjs (Browser/Node client)
20 stars 10 forks source link

Changed `_onConnect` to use a regular function instead of an arrow function. #7

Closed jonwrona closed 6 years ago

jonwrona commented 6 years ago

Using the arrow function looked like it was breaking the this context for the _onConnect function. Changing this to a regular function fixed this for my local project.

dmonad commented 6 years ago

Hi @jonwrona For me the two implementations look identical. The function is called as this._onConnect() in the connector, so the context will always be set to the connector instance. I usually use arrow functions to explicitly state in which context I want the function to be executed. So I'd prefer to use the arrow function. But I'd like to find out more about your issue. Does your transpiler throw warnings?

jonwrona commented 6 years ago

@dmonad

It may be a function hoisting issue with the transpiler. I'm using webpack + babel. When I attach yWebsocketsClient to the Y instance using the npm package, it throws Uncaught Error: Expected a function!, which seemed strange, so I copied the contents of the file to a local version. When importing the local js file instead of the package's, the error becomes Uncaught ReferenceError: options is not defined, with options referring to line 23, the first usage inside of the this._onConnect function. For some reason when I changed that function from:

this._onConnect = () => { to this._onConnect = function() {

it started running without error.

jonwrona commented 6 years ago

In my mind there is no difference between the two functions, but for some reason after my build process using a regular function makes the difference. Is there any other information that I could grab for you to help?

jonwrona commented 6 years ago

@dmonad I think I discovered why this is an issue. I may be incorrect about this but it seems that arrow functions use the this from the enclosing scope, which should be the class, but because you are wrapping the class in a function, what may be happening is the scope of the function is getting applied to the _onConnect function, so inside the function the wrong this is being referenced.

The this of arrow functions is the nearest function scope, meaning the scope of the function defined using the function keyword in which it occurs. (source)

dmonad commented 6 years ago

Can you show me your babel config and babel version?

jonwrona commented 6 years ago

babel-core: 6.26.0

.babelrc

{
  "presets": [
    ["es2015", {"modules": false}],
    "react"
  ],
  "plugins": [
    ["babel-plugin-root-import"],
    "transform-es2015-spread",
    "transform-es2015-modules-commonjs",
    "import-glob",
    "react-hot-loader/babel",
    "transform-react-inline-elements",
    "transform-react-constant-elements",
    "transform-async-to-generator"
  ]
}
dmonad commented 6 years ago

Hey @jonwrona

I can't reproduce this problem with babel. So this is probably a webpack issue.

I really would like to find out the real problem. I changed it to a simple function for now and Implemented your PR (see above), but removed this statements in the function.

9.0.0-4

But I'd also recommend you to use the node build if you run into this problem a lot. In Yjs this is really simple:

instead of import 'y-websockets-client' write import 'y-websockets-client/y-websockets-client.node.js'. Otherwise your module bundler will grab 'yjs/src/y-websockets-client.js' as it thinks it can handle es6 code.