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

refactor: replace console.warn with process.emitWarning #954

Merged
merged 1 commit into from Apr 16, 2022
Merged

refactor: replace console.warn with process.emitWarning #954

merged 1 commit into from Apr 16, 2022

Conversation

lamweili
Copy link
Contributor

@lamweili lamweili commented Apr 8, 2022

To have consistent warnings from #953 (comment) by @JPeer264

Yes process.emitWarning sounds also like a great option to add (just my opinion on it). However, a separate PR would be nice if you want to go the extra mile as we squash our PRs. @manidlou @RyanZim @jprichardson any objections?

Prints out via process.emitWarning in the following format:

image

@lamweili lamweili marked this pull request as draft April 8, 2022 17:21
@lamweili lamweili marked this pull request as ready for review April 8, 2022 17:53
@lamweili lamweili marked this pull request as draft April 8, 2022 17:55
@lamweili lamweili marked this pull request as ready for review April 8, 2022 17:55
Signed-off-by: Lam Wei Li <peteriman@mail.com>
@RyanZim
Copy link
Collaborator

RyanZim commented Apr 8, 2022

Should we consider this: https://nodejs.org/api/process.html#avoiding-duplicate-warnings?

@lamweili
Copy link
Contributor Author

lamweili commented Apr 8, 2022

My motivation was to retain the behavior of the console.warn which warns duplicatedly if called multiple times.

Should we change it to use https://nodejs.org/api/process.html#avoiding-duplicate-warnings instead, so it only warns on the first time?

What do you guy think? Let me know and I'll make the adjustments. 😊
@RyanZim @JPeer264 @manidlou @jprichardson

Copy link
Collaborator

@JPeer264 JPeer264 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with either. Thanks for the changes and your efforts :)

@RyanZim RyanZim merged commit baa9934 into jprichardson:master Apr 16, 2022
@lamweili lamweili deleted the refactor/warning-mechanism branch April 17, 2022 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants