feat(server): add serverMode option #1937
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1937 +/- ##
=========================================
- Coverage 91.8% 91.8% -0.01%
=========================================
Files 22 23 +1
Lines 879 903 +24
Branches 276 283 +7
=========================================
+ Hits 807 829 +22
- Misses 69 71 +2
Partials 3 3
Continue to review full report at Codecov.
|
lib/Server.js
Outdated
| 'via require.resolve(...), or the class itself which extends BaseServer' | ||
| ); | ||
| } | ||
| this.ServerImplementation = ServerImplementation; |
There was a problem hiding this comment.
- Let's move this to util
getSocketServerImplementation - Don't think we need
function - Let's rewrite this on
switch
There was a problem hiding this comment.
@evilebottnawi function option is good if the user wants to supply an implementation, but their file is exporting multiple things, for example. So like if they want to do:
serverMode: require('./my-implementation').Server
There was a problem hiding this comment.
I think better use require.resolve('./my-implementation/Server')
/cc @hiroppy
There was a problem hiding this comment.
I'm fine either way:) Currently, it was fixed as require.resolve.
test/options.test.js
Outdated
| @@ -340,6 +341,15 @@ describe('options', () => { | |||
| success: [true], | |||
| failure: [''], | |||
| }, | |||
| serverMode: { | |||
| success: [ | |||
| '', | |||
| config, | ||
| { | ||
| sockPath: '/foo/test/bar/', | ||
| serverMode: class MySockJSServer extends BaseServer { |
There was a problem hiding this comment.
@evilebottnawi being able to pass in class also makes testing easier
|
|
||
| if (!serverImplFound) { | ||
| throw new Error( | ||
| "serverMode must be a string denoting a default implementation (eg. 'sockjs'), a full path to " + |
lib/Server.js
Outdated
| ); | ||
| } | ||
|
|
||
| this.ServerImplementation = getSocketServerImplementation(this.options); |
There was a problem hiding this comment.
I think we should rename variable to this.socketServerImplementation to avoid misleading, we already store value in this.socketServer so let's use socket prefix too
There was a problem hiding this comment.
I'm going to capitalize first letter to clarify that it is a class.
lib/Server.js
Outdated
| } | ||
|
|
||
| // this.SocketServerImplementation is a class, so it must be instantiated before use | ||
| this.SocketServerImplementation = getSocketServerImplementation( |
There was a problem hiding this comment.
this.SocketServerImplementation -> this.socketServerImplementation
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
The goal of this option is to allow the user to choose the current server implementation, or provide their own implementation as a path/class.
It is a GSoC Refactor goal: #1860
Breaking Changes
None
Additional Info