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

Allow debug to accept same args at prettyDOM #580

Closed
hottmanmichael opened this issue Jan 28, 2020 · 7 comments · Fixed by #596, eapenzacharias/Calculator-REACT#13, guram21/cra1#1 or guram21/counter-app#1 · May be fixed by mantonionip/nasa-flickr-gallery#2
Labels

Comments

@hottmanmichael
Copy link

hottmanmichael commented Jan 28, 2020

Describe the feature you'd like:

When using the debug wrapper in a test, it would be useful to be able to pass additional prettyDOM arguments through from debug

An example use case is being able to extend the number of lines that prettyDOM allows without needing to specify this in env prior to running the tests or needing to use prettyDOM directly

Example:

it('should print all the lines', () => {
    const {  debug } = render(<LargeComponent />);
    const maxLinesForThisTest = 10000;
    debug(undefined, maxLinesForThisTest);
});

I'd be happy to implement this feature, however just curious if there was a specific reason why this was not implemented on the first pass of debug - My team is familiar with using debug for printing out the DOM and we don't often use prettyDOM, and since this wrapper already exists as an api of the render method, allowing users to use debug as the default api makes sense to me

Suggested implementation:

1. First Suggestion:
Additional arguments could be forwarded through debug and passed into prettyDOM in this chunk of code

debug: (el = baseElement) =>
Array.isArray(el)
? // eslint-disable-next-line no-console
el.forEach(e => console.log(prettyDOM(e)))
: // eslint-disable-next-line no-console,
console.log(prettyDOM(el)),
(I believe this is the correct place after a little bit of investigation, but am open to other implementations)

debug: (el = baseElement, maxLength, options) => 
   Array.isArray(el) 
     ? // eslint-disable-next-line no-console 
       el.forEach(e => console.log(prettyDOM(e,  maxLength, options))) 
     : // eslint-disable-next-line no-console, 
       console.log(prettyDOM(el, maxLength, options)), 

2. Second Suggestion
Keep the implementation of debug the same and add an additional method that always prints the baseElement and allows the user to forward args to prettyDOM - I'm not a big fan of this solution, but 99% of time I've found myself calling debug() without any arguments with the assumption that I wasn't able to pass debug(someElement) as the first argument - so being able to call debug(maxLines) without specifying the debugged element might be nice

debugBaseElement: (maxLength, options) => 
   const el = baseElement;
   Array.isArray(el) 
     ? // eslint-disable-next-line no-console 
       el.forEach(e => console.log(prettyDOM(e,  maxLength, options))) 
     : // eslint-disable-next-line no-console, 
       console.log(prettyDOM(el, maxLength, options)), 

Describe alternatives you've considered:

  • It's possible to get add additional lines by setting an env in the command line
    DEBUG_PRINT_LIMIT=10000 npm test however when running jest in --watch mode, this requires us to kill the test runner, update the print limit, then rerun tests which isn't super painful, but adding this inline would be faster
  • Also possible to use prettyDOM directly from @testing-library/dom, however it's not immediately clear from the docs whether we should be passing in container, asFragment(), or baseElement into this method - Also, since debug is already a wrapper on top of prettyDOM, it makes sense to be able to pass the same args IMO
    • Another downside of prettyDOM is that debug wraps prettyDOM in a console.log, whereas prettyDOM requires you to call prettyDOM within a console.log, which makes using the two apis interchangeably potentially confusing

Teachability, Documentation, Adoption, Migration Strategy:

Docs would stay very much the same, only adding a link to prettyDOM from https://testing-library.com/docs/react-testing-library/api#debug and ensure that it's clear the user can forward prettydom arguments from debug

@kentcdodds
Copy link
Member

Hi there @hottmanmichael,

Thanks for the issue. I'm good with your first suggestion :) Would you like to make a pull request?

@hottmanmichael
Copy link
Author

@kentcdodds Yep! Would be happy to 👍 Thanks for the quick reply! This'll be my first open source contribution

@kentcdodds
Copy link
Member

Awesome!

@kentcdodds
Copy link
Member

Hope you don't mind that I was working on things today and decided to implement this myself. Thanks for the idea though!

@kentcdodds
Copy link
Member

@all-contributors please add @hottmanmichael for ideas

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @hottmanmichael! 🎉

@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 9.5.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