-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
WIP: Make console.error and warn log a stack trace and codeframe like thrown errors do #9741
Conversation
Hi @ychi! Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
8c6c2d5
to
7fe4f36
Compare
7fe4f36
to
042d872
Compare
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Hi @SimenB, Can you take a look at the proposed output in the Summary above when you are available? I think many snapshot tests need to be updated accordingly. Please let me know which areas I can improve the code, too. Thank you very much! |
Oh yeah, this is looking good! I like it 👍 Let's drop the |
@gaearon thoughts on the output here? |
I think this is shaping up great @ychi! 👍 Could you add some tests, update the snapshots and add an entry to the changelog? |
a4f6158
to
6d4322e
Compare
packages/jest-console/package.json
Outdated
"chalk": "^3.0.0", | ||
"jest-util": "^25.3.0", | ||
"jest-message-util": "^25.2.6", | ||
"jest-types": "^25.2.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"jest-types": "^25.2.6", | |
"@jest/types": "^25.3.0", |
and at the top.
jest-util
is also the wrong version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reminding about this! dependencies
in package.json
should always use package name while references
in tsconf.json
would be relative path.
By the way, what's the reason some package names are scoped while others are not? Is it because packages published in the early days and not wanting to change package name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We got the jest namespace pretty late, and to avoid confusion we haven't renamed any packages. So in general old packages are not scoped, but new ones are scoped
No need to pass SourceMapRegistry
Thanks @ychi! |
Oh! So much going on in one day. Thank you very much @SimenB for helping with so many things in this PR! |
@SimenB Sorry, but I'm not interested in those stacktraces, they only add noise in my case. Can you please reconsider adding |
We have a |
Generally, I do, but I think I'll try to live with this option a bit, thanks. UPD: it doesn't seem to affect these console stacktraces |
Oh, that's a bug I think |
I can look into the bug later today :) |
It's because One way I can see would be like this:
In which Should we go with this route? Would it be suitable for the next minor release? |
It should respect |
hi, in my case it adds noise as well. How do i get rid of console errors and warn? Thanks |
If there are errors/warn in your console, you should find where those are appearing (which this new feature should help with) and why, and change your code so those don't show up. It's dangerous to simply hide errors and warnings. They probably exist for a reason and are indicative of a problem. |
Im aware of that but i want to have the option to not include them in my tests. dont want to change my source code for that.. |
If you want to get rid of the stack trace, then it sounds like the next version will support that with console.error = () => {}
console.warn = () => {} I don't advise this for the reasons I mentioned earlier :) |
thanks! |
Why does it also apply to |
Call sites were moved to a different line for all console methods, while |
I'd say that this IS a stacktrace, just reduced to one line. And it definitely doesn't help readability |
Hi guys, |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Make
console.error
andconsole.warn
log a stack trace like thrown errors do. As requested in this issue. Visual examples:Error used to be like this:
It will become like this:
What warn used to look like:
And will become:
There won't be stack trace for
console.log
, but for consistency, it's proposed to print like this:Instead of this:
In the original feature request, there were discussion possibility to opt out. This can be achieved by exposing a new cli option
--noCodeFrame
.Closes #8819
Test plan
To pass CI, some snapshots will need to be updated.