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

Don't use console.error #456

Closed
yulianovdey opened this issue Apr 19, 2023 · 3 comments · Fixed by #508
Closed

Don't use console.error #456

yulianovdey opened this issue Apr 19, 2023 · 3 comments · Fixed by #508

Comments

@yulianovdey
Copy link

Describe the bug
There is a console.error call here we can't route through our logging handler. This is particularly an issue with Datadog because err.stack ends up including new lines and every line ends up as a separate log statement.

To Reproduce
We see this happen whenever a server error is returned from Nylas.

Expected behavior
nylas-nodejs does not log anything directly, and instead offers an option to toggle this behavior, or provides a way to inject a logger.

SDK Version:
Latest.

Additional context
It looks like calls to console.log were removed in 3.1.1, but this error log was kept in.

Thank you!

@mrashed-dev
Copy link
Collaborator

Hey @yulianovdey! Thanks for opening this issue. We can remedy this by removing that console.error line and instead we can just intercept the error and throw an SDK specific error with the original message embedded with it. We'll fix this and include it in a future update. Thanks for opening this issue!

@relaxedtomato
Copy link
Contributor

approach to fix:

  • Need to remove all instance of console.error lines
  • Add linting rule to prevent console.errors from being added to the code base
  • Create a new error called NylasSdkError
  • Replace the console.error indicated by the user with

@mrashed-dev
Copy link
Collaborator

Thanks for your patience @yulianovdey, we now remove the console calls and allow you to pass in a logger if you want to receive messages.

mrashed-dev added a commit that referenced this issue Nov 28, 2023
This PR adds in an option to pass in a logger instead of the SDK using console to log. Closes #456.
mrashed-dev added a commit that referenced this issue Nov 29, 2023
# Changelog
* Add support for logging (#508, #456)
* Nullify replyToMessageId is an empty string (#484, #509)
* Nullify visibility if visibility is an empty string (#507, #470)
* Fix numbers defaulting to 0 instead of null (#469)
* Fix parsing of Number arrays (#503, #502)
* Fix configured timeout not being used (#506, #489)
* Bump `node-fetch` dependency from 2.6.1 to 2.6.12 (#504, #496)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants