Simplify the selection of the dynamic port in the nodejs sdk. #1128
Conversation
|
Build Succeeded 👏 Build Id: 5fabe9d0-a195-4fb9-9488-109ee0200120 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pooneh-m, roberthbailey The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -25,22 +25,8 @@ class AgonesSDK { | |||
| } | |||
|
|
|||
| get port() { | |||
| const defaultPort = '59357'; | |||
| const port = process.env.AGONES_SDK_GRPC_PORT; | |||
There was a problem hiding this comment.
In JavaScript it's common to use || e.g. do return process.env.AGONES_SDK_GRPC_PORT || '59357'
Without examining grpc code too much it seems port should be a number so you could cast using return Number(process.env.AGONES_SDK_GRPC_PORT) || 59357
There was a problem hiding this comment.
doh too slow :)
There was a problem hiding this comment.
I'm happy to fix it in a follow-up PR. :)
There was a problem hiding this comment.
it seems port should be a number
The value is returned and concatenated with 'localhost' so I think returning a string is ok.
markmandel
added
area/user-experience
This brings the NodeJS SDK in line with the C++ / Unity / Rust SDKs.
Part of #851.