lib: refactor remoteProxy using classes #269
Conversation
lib/remote-proxy.js
Outdated
| @@ -1,69 +1,64 @@ | |||
| 'use strict'; | |||
|
|
|||
| const events = require('events'); | |||
| const util = require('util'); | |||
| const EventEmitter = require('events').EventEmitter; | |||
There was a problem hiding this comment.
You should use destructuring assignment here to keep the style consistent.
lib/remote-proxy.js
Outdated
|
|
||
| // Create a method in a remote proxy that will call the corresponding remote | ||
| // method. This is implemented as a static method rather than an instance |
There was a problem hiding this comment.
This is implemented as a static method rather than an instance method so that it will not be rewritten by a remote API method with the same name.
Why did you change it?
I think that reasoning here is clear and it should still be a static method after refactoring.
There was a problem hiding this comment.
It is okay to make it an instance method. Nothing bad will actually happen even if it gets overwritten. The possibility of having emit and on methods in an interface is much more concerning.
UPD: actually it can be overwritten before other methods are wrapped in the constructor. It can be mitigated via const wrap = this._wrapRemoteMethod.bind(this); and calling the wrap function inside the loop though, or even moving the function out of the class and making it taking a proxy instance as its argument.
nechaido
force-pushed
the
refactoring/remote-proxy
branch
from
a088fb7
to
5ca972b
Compare
nechaido
force-pushed
the
refactoring/remote-proxy
branch
from
5ca972b
to
7a5380c
Compare
PR-URL: #269 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
|
Landed in de38e27. |
PR-URL: #269 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
PR-URL: #269 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
No description provided.