Fix #496 Add clientOptions.logger option (and improvements to other attributes too) #856
Conversation
Codecov Report
@@ Coverage Diff @@
## main #856 +/- ##
==========================================
- Coverage 65.98% 65.87% -0.11%
==========================================
Files 13 13
Lines 1176 1184 +8
Branches 344 348 +4
==========================================
+ Hits 776 780 +4
- Misses 334 336 +2
- Partials 66 68 +2
Continue to review full report at Codecov.
|
| if (agent !== undefined && this.clientOptions.agent === undefined) { | ||
| this.clientOptions.agent = agent; | ||
| } | ||
| if (clientTls !== undefined && this.clientOptions.tls === undefined) { | ||
| this.clientOptions.tls = clientTls; | ||
| } |
There was a problem hiding this comment.
In the past versions, clientOptions.agent was overwritten if agent exists in the App constructor arguments. This is the same for clientOptions.tls vs clientTls.
src/App.ts
Outdated
| this.clientOptions.tls = clientTls; | ||
| } | ||
| if (logLevel !== undefined && logger === undefined) { | ||
| // If the constructor has both, logLevel is ignored |
There was a problem hiding this comment.
I don't think this can affect many existing apps but this is for keeping backward compatibility (just in case).
There was a problem hiding this comment.
Should we warn the developer in this case in case there's confusion?
| this.clientOptions.logLevel = logLevel; | ||
| } else { | ||
| // Since v3.4, WebClient starts sharing loggger with App | ||
| this.clientOptions.logger = this.logger; |
There was a problem hiding this comment.
The visible change would be the name of logger. I think it's better for most apps.
seratch
changed the title
Summary
This pull request resolves #496 by passing
loggertoWebClientinstance. Also, I've improved the initialization of other attributes.Requirements (place an
xin each[ ])