Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.5 Samples]: Remove logger from the Node.js samples #1465

Closed
stevengum opened this issue Apr 30, 2019 · 3 comments · Fixed by #1606
Closed

[4.5 Samples]: Remove logger from the Node.js samples #1465

stevengum opened this issue Apr 30, 2019 · 3 comments · Fixed by #1606
Assignees
Labels
bug Indicates an unexpected problem or an unintended behavior.

Comments

@stevengum
Copy link
Member

Branch: samples-work-in-progress

Sample information

  1. Sample type: samples
  2. Sample language: JavaScript & TypeScript Node.js samples
  3. Sample name: All applicable samples

Describe the bug

The wrapper around console (i.e. const logger = console;) is superfluous. We're going to back it out and instead call console.log() or the applicable console method as needed.

logger as an argument will not be passed to the Bot constructors, nor will it continue to exist.

To Reproduce

Steps to reproduce the behavior:

  1. Go to Core bot in JavaScript or TypeScript
  2. Open the index.js/ts for thesample
  3. See the line with const logger = console;
  4. Pause
  5. Look again
  6. Be baffled

[bug]

@stevengum stevengum added the 4.5 label Apr 30, 2019
@stevengum stevengum changed the title [4.4 Samples]: Remove logger from the Node.js samples [4.5 Samples]: Remove logger from the Node.js samples Apr 30, 2019
@v-kydela
Copy link
Contributor

v-kydela commented May 1, 2019

May ask for some clarification?

console is also assigned to logger in the DialogBot and MainDialog classes. Is the logger only going to be removed from index.js/ts or is it going to be removed from everywhere?

My understanding was that the logger argument served the purpose of parity with the C# samples, and also the purpose of allowing something other than console to be provided for logging, as indicated by the comment: // Pass in a logger to the bot. For this sample, the logger is the console, but alternatives such as Application Insights and Event Hub exist for storing the logs of the bot.

Is this no longer needed?

@sgellock sgellock self-assigned this May 9, 2019
@sgellock sgellock added 4.5 bug Indicates an unexpected problem or an unintended behavior. and removed 4.5 labels May 9, 2019
@cleemullins cleemullins assigned tracyboehrer and unassigned sgellock Jun 27, 2019
@tracyboehrer
Copy link
Member

tracyboehrer commented Jun 27, 2019

What is the expected change for this issue? I have the same question as Kyle.

Does this mean it's expected to remove the logger constructor arg, and replace calls to it in each bot with a call to console.log?

@sgellock
Copy link
Member

Yes. We were being too clever. let's just use console.log as it's the only logger we use in the js/ts samples

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or an unintended behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants