fix: proxy event local variable #1655
Conversation
|
Hi @akoserwal, thanks for your first time PR! 🎉 🎉 This approach LGTM! Any concerns @hekike? |
lib/server.js
Outdated
| @@ -138,6 +130,12 @@ function Server(options) { | |||
| var fmt = mergeFormatters(options.formatters); | |||
| this.acceptable = fmt.acceptable; | |||
| this.formatters = fmt.formatters; | |||
| this.proxyEvents = new Array('clientError', | |||
There was a problem hiding this comment.
Please use the shorthand syntax: new Array() => []
| @@ -187,10 +185,10 @@ function Server(options) { | |||
|
|
|||
| this.router.on('mount', this.emit.bind(this, 'mount')); | |||
|
|
|||
| if (!options.handleUpgrades && PROXY_EVENTS.indexOf('upgrade') === -1) { | |||
| PROXY_EVENTS.push('upgrade'); | |||
| if (!options.handleUpgrades && this.proxyEvents.indexOf('upgrade') === -1) { | |||
There was a problem hiding this comment.
@akoserwal now that proxyEvents in retrospect I don't think we even need the indexOf check - is that right?
|
Yep, looks good to me. |
|
@hekike : done |
lib/server.js
Outdated
| @@ -187,10 +187,10 @@ function Server(options) { | |||
|
|
|||
| this.router.on('mount', this.emit.bind(this, 'mount')); | |||
|
|
|||
| if (!options.handleUpgrades && PROXY_EVENTS.indexOf('upgrade') === -1) { | |||
| PROXY_EVENTS.push('upgrade'); | |||
| if (!options.handleUpgrades && this.proxyEvents.includes === false) { | |||
There was a problem hiding this comment.
Do you mean this.proxyEvents.includes('upgrade') === false? That said, I don't think node 4 supports the includes method. @hekike we may have to stick with indexOf here for now?
There was a problem hiding this comment.
You are right, apologize for extra work :/
|
Hi @akoserwal - thanks for the contribution! Would you mind adding unit tests to round out the PR? |
|
@DonutEspresso: Please can you review again. I did |
|
@hekike did we setup the make file to run prettier? What's the best way to autofix that stuff? |
| handleUpgrades: true | ||
| }); | ||
|
|
||
| t.equal(server.proxyEvents.length,6); |
There was a problem hiding this comment.
I think I expect handleUpgrades: true to be 7, and 6 for false (may be related to the && indexOf check above)
|
please review #1661: @DonutEspresso @hekike |
Pre-Submission Checklist
make prepushIssues
#1348
Closes:
Since PROXY_EVENTS are shared between every instance of a restify server, and upgrade is added but never removed, all future instances of Server will have the upgrade event proxied.
I haven't added unit-test. As this is my first PR. I like to get initial review. before working on the unit test.
Changes