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

show line in debug() #440

Closed
benmonro opened this issue Jan 31, 2020 · 15 comments · Fixed by #733
Closed

show line in debug() #440

benmonro opened this issue Jan 31, 2020 · 15 comments · Fixed by #733
Labels
enhancement New feature or request help wanted Extra attention is needed released

Comments

@benmonro
Copy link
Member

Describe the feature you'd like:

I'm not sure if this is even possible, but figured I'd throw it out there. When I use console.log('foo') the console will include the line number where the console log was made. However, when using debug() it's always pointing to the line in dom-testing-library (see screenshot).
image

I'm not even sure if its possible to do this, but it would be so great if instead showed the line in my code where debug was called.

Suggested implementation:

Describe alternatives you've considered:

Teachability, Documentation, Adoption, Migration Strategy:

@kentcdodds
Copy link
Member

That would be awesome. I don't think it's possible to update the Jest stack trace from our perspective. Jest overrides console to enable this. Maybe we could do something to our log to include your line ourselves. Something like:

const firstUserCodeLine = new Error()
  .stack
  .split('\n')
  .find(line => !line.includes('node_modules')) || ''

Then we could log that first (and make it gray like they do). I'd be open to a PR.

Also, maybe someone could do some investigation in Jest to see if there's some way to change which stack trace line gets logged with the console.log call.

@eps1lon
Copy link
Member

eps1lon commented Feb 1, 2020

Basically what Kent is saying in #440 (comment). But I would prefer to just slice out our particular frame instead of relying on n_m layout.

@kentcdodds kentcdodds added enhancement New feature or request help wanted Extra attention is needed labels Feb 2, 2020
@victorandcode
Copy link
Contributor

Hello, I'd like to take a look at this. @eps1lon I've thought about the extraction of a particular frame, however, since prettyDOM is also used in other libraries such as @tesing-library/react I'm not sure if we can differentiate between frames from the particular instance of the @testing-library and client code without relying on node_modules. I've created a draft solution here victorandcode@39bcde4
It's still rough around the edges and is missing the grey coloring but I'm happy to hear what you think :) . A couple of things to mention:

@eps1lon
Copy link
Member

eps1lon commented Mar 25, 2020

Maybe this is naive but I literally meant .slice(). Does transpiling or dev vs prod affect that?

@victorandcode
Copy link
Contributor

Sorry, I'm not sure if I understand what you mean by "affect that". Do you mean the fact that we can't differentiate between client and library code without relying on node_modules? In that case, I believe it's with prod builds. If you're referring to removing internal, same case. If you're referring to the grey coloring, it's just missing for now until we find a possible solution 😃

@kentcdodds
Copy link
Member

You know... If this happens: jestjs/jest#8819

Then we could just switch to console.error and change the color so it's not red 🤔

@delca85
Copy link
Member

delca85 commented Aug 3, 2020

Hi!
I don't know if @victorandcode is still on this one because otherwise I would be glad to work on it.

@victorandcode
Copy link
Contributor

Hello, I’d love to keep working on this now that the jest issue is solved. I’ll take a look these next couple of days.

@victorandcode
Copy link
Contributor

Here are the 2 alternatives I've found to implement this

Option A

We use the new console.warn or console.error from jest, which contains the call stack. Here's a sample:

Screenshot from 2020-08-05 20-45-56

The problem with this approach is that it's verbose and the stack frame we care about isn't obvious (here it's src/app.test.js:22:12).

Option B

If we do the heavy lifting ourselves with something like this commit.

Screenshot from 2020-08-05 20-51-40

Unfortunately, the line number doesn't play well with transpilers so it can easily be wrong. I tested this with a new babel-jest project and it had this issue.

Of course, I lean towards the first option, but for me it adds too much noise so I'd prefer not to have it. If we find a way around the second option's issue I'd prefer that one.

Would love to know your thoughts @eps1lon @kentcdodds

@benmonro
Copy link
Member Author

benmonro commented Aug 5, 2020

either option is definitely a huge improvement over current situation. having said that, if i had to choose, i probably would prefer option 2, but if the reasons you mention are problematic, i'd be ok w/ option 1. but just curious, is there an option 3... i.e. option 1 + option 2 both?

@victorandcode
Copy link
Contributor

I'd say they're mutually exclusive. If the second one worked without additional work (e.g. babel config), then the first one wouldn't be needed at all.

🆘 If anyone knows of an easy way to make option 2 work (a.k.a. how to configure babel and typescript to show the correct line for ES6 in a jest environment), we could follow that one. I might be missing something.

I'll wait a bit (1-2 days) and if there's no other suggestion I'll work on the first option :)

@benmonro
Copy link
Member Author

benmonro commented Aug 5, 2020

Works for me. option 1 is a welcome improvement to me.

@kentcdodds
Copy link
Member

I wonder if we could print the stack trace and code frame ourselves. I'm pretty sure we could get the stack trace and then hand that to some module to retrieve the code frame which we could log. Want to investigate that?

@victorandcode
Copy link
Contributor

I ended up going with Kent's approach on this one. Just had to add a bit of coloring and it just shows the line where screen.debug was called.
Let me know what you think when you have some time.

@testing-library-bot
Copy link

🎉 This issue has been resolved in version 7.24.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants