Improve debug-level logging in InstallProvider#handleInstallPath() #1451
Conversation
seratch
added
bug
| } catch(error) { | ||
| console.log(error) | ||
| } | ||
| await installer.handleInstallPath(req, res, {}, { |
There was a problem hiding this comment.
Updated the example app for verifying the behavior.
| @@ -330,7 +330,10 @@ export class InstallProvider { | |||
| } | |||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |||
| const _installOptions: InstallURLOptions = installOptions! || this.installUrlOptions!; | |||
| this.logger.debug(`Running handleInstallPath() with ${_installOptions}`); | |||
| if (this.logger.getLevel() <= LogLevel.DEBUG) { | |||
| const _printableOptions = JSON.stringify(_installOptions); | |||
There was a problem hiding this comment.
This is the essential change in this PR
| userScopes: ['chat:write'], | ||
| metadata: 'meta', | ||
| }; | ||
| assert.deepEqual(JSON.stringify(options), '{"teamId":"T111","redirectUri":"https://www.example.com/slack/oauth_redirect","scopes":["commands","chat:write"],"userScopes":["chat:write"],"metadata":"meta"}'); |
There was a problem hiding this comment.
just in case, added a simple test to make sure if the object does not have any issues with JSON.stringify
| } catch(error) { | ||
| console.log(error) | ||
| } | ||
| await installer.handleInstallPath(req, res, {}, { |
| @@ -330,7 +330,10 @@ export class InstallProvider { | |||
| } | |||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |||
| const _installOptions: InstallURLOptions = installOptions! || this.installUrlOptions!; | |||
| this.logger.debug(`Running handleInstallPath() with ${_installOptions}`); | |||
| if (this.logger.getLevel() <= LogLevel.DEBUG) { | |||
There was a problem hiding this comment.
I don't think we need this conditional here. The Logger class has this check built-in already. I think we can remove the if.
There was a problem hiding this comment.
@filmaj I've added this if condition to avoid the cost to run JSON.stringify when the logging level INFO or above. That being said, we may want to simplify the logic rather than the slight efficiency improvement.
There was a problem hiding this comment.
this is an ignorable cost. removed the if condition
seratch
force-pushed
the
oauth-debug-log
branch
from
2fef094
to
2adb3e5
Compare
Summary
This pull request improves a debug-level logging in
InstallProvider#handleInstallPath()method. We can release a patch version including this improvement.Requirements (place an
xin each[ ])