Getroles logroles

[michaellasky]

This is a draft implementation for the functionality requested in #282

The intention is to give a jumping off point for discussion.

I've added a file named role-helpers.js which implements the two requested functions, and also moved the function buildElementRoleList from src/queries/role.js into role-helpers.js. Since role-helpers needed similar functionality, and it didn't seem to make sense to export it from query/role with a bunch of query functions.

I also added a dependency on lodash.merge for a deep merge in getRoles. Maybe this could be removed because the merge happening there isn't all that complicated. But using the premade merge() provided by lodash allowed me to keep the initial PR code cleaner for discussion.

Checklist:

• Documentation added to the
  docs site
• Typescript definitions updated
• Tests
• Ready to be merged
  

I'm still pretty new to this particular code base, so I don't think it's ready to be merged without some significant review and iteration from more experienced members.

[michaellasky]

I have code for a logRoles test that doesn't rely on snapshotting if that's easier for the CI (referencing the failed build). I'm using a snapshot because the test shouldn't necessarily fail (beyond needing to update the snapshot) if the formatting (magic that makes crazy console colors) from debugDOM changes. If I hardcode the output from debugDOM into a test case, then the code would need to be updated if debugDOM formatting changes.

A snapshot makes sense as well since logRoles just returns a string that is formatted to loosely defined, visually pleasing standards. IMO we don't need to test every little aspect of that (where every newline or space is, etc) It's self evident if it doesn't look nice in everyday usage.

Edit: Thinking about this further I think that test certainly needs some checking to make sure the string at least contains the expected node and role names.

Updated test, no snapshot.

[kentcdodds]

Thanks for this! It's pretty hard to visualize what this would look like. I think a snapshot would be appropriate for the logRoles function test.

Also, a screenshot would be helpful!

Thanks for this!

[michaellasky]

Also, a screenshot would be helpful!

Quick screenshot from npm test with a console log I'll spin it up in the browser in a bit when I make the other changes and take a look at / screencap the output there.

I can of course make any changes to the formatting. I didn't match the suggested implementation exactly, but can if that's wanted.

[kentcdodds]

Looks pretty good to me!

1. It would probably be helpful to indent the DOM output so the sections are more visible
2. It would probably be helpful to have a separator (even just a single line) between each DOM output so people don't think they represent the same node or something.
3. That <article> node has a bunch of stuff in it and if it were really big, that output could be VERY extensive. I wonder if we should just clip it to <article /> instead of <article>all the contents</article> Thoughts?

[michaellasky]

3. That `<article>` node has a bunch of stuff in it and if it were really big, that output could be VERY extensive. I wonder if we should just clip it to `<article />` instead of `<article>all the contents</article>` Thoughts?

I actually had a similar thought. I didn't pursue it because that would be more of a change to debugDOM I think? We'd need to add a parameter to tell it not to print all the children (assuming it can't already do that, i haven't looked at it extensively). The current implementation is just debugDom(node), which in the case of a large node like the article node, prints out all the children.

In any event, i don't think it'd be very difficult to handle.

[kentcdodds]

My thinking is that before we hand it to debugDOM we could create a copy of the DOM node (via something similar to asFragment) and then remove all the children.

[michaellasky]

Oh, I hadn't considered that. That's not a bad idea, I'll give it a shot and update this (hopefully tonight, time permitting) with that and the other suggested changes.

[michaellasky]

So I've made most of the discussed changes.

This is what the output of logRoles looks like now

I also fixed a bug with getRoles, and refactored it to clean it up and simplify things.

logRoles uses a snapshot test again. The regex only worked if the html under test is very simple, and it a tad more 'real world'. So rather than putting an ugly regex in the test i went back to a snapshot.

Lodash.merge dependency is gone.

[kentcdodds]

LGTM! Would love another reviewer on this though.

[michaellasky]

I've attempted to add the typescript definitions but I seem to be doing something wrong. I'm not particularly fluent with typescript.

I'd imagine the definitions, in a file named typings/role-helpers.d.ts, should look something like:

export function logRoles(container: HTMLElement): string
export function getRoles(
    container: HTMLElement,
 ): {[index: string]: HTMLElement[]}

But unfortunately that fails linting (kcd-scripts) on build with:

2:1  error  Parsing error: Unexpected token, expected "{"

After some experimentation, I can only get it to pass if I add an implementation. For debugging purposes, I changed it to:

export function logRoles(container: HTMLElement): string
export function getRoles(container: HTMLElement): string

Which fails in the same way. But:

export function logRoles(container: HTMLElement): string { return container? '1': '2' }
export function getRoles(container: HTMLElement): string { return container? '1': '2' }

passes linting fine. I am confuse. But it's probably just some assumption I'm making because of lack of TS experience.

Full Error output:

✖ kcd-scripts lint found some errors. Please fix them and try committing again.

/home/mlasky/dev/dom-testing-library/typings/role-helpers.d.ts
2:1  error  Parsing error: Unexpected token, expected "{"


  1 | export function logRoles(container: HTMLElement): string
> 2 | export function getRoles(
    | ^
  3 |   container: HTMLElement,
  4 | ): {[index: string]: HTMLElement[]}
  5 | 

 ✖ 1 problem (1 error, 0 warnings)

[kentcdodds]

Yeah, honestly I'm not sure what's up with that 😞 we should probably just disable linting on that file. This always happens to me when I change that file so I just commit that file with --no-verify 😬

I think things look pretty good. I'd love another review though!

[kentcdodds]

Oh, and it looks like there's a merge conflict 😬

[michaellasky]

Added the typescript definitions. Whoever else reviews these changes should pay particular attention to them, since I don't have complete confidence they're correct.

93f9530

[alexkrolick]

As far as I know there aren't any aria-roles greater than 42 chars, but I would really hate for a formatting error to break someone's test. Can we be conservative here and do a Math.min(0, numDelimiters) or move the delimiter characters to a fixed length line?

[michaellasky]

That's a good point. I think it's very unlikely there will ever be a role greater than 42 characters long... there's also no real downside to protecting against that. Maybe I was being too clever with embedding the name in the delimiters. It read easier (to my eyes) with just a single heading/spacer element, but it probably makes more sense to just implement it like Kent originally had it and avoid any confusion, extra code, or bugs (however unlikely).

I'll make that change this evening unless I hear an objection.

[michaellasky]

Done 593c33f

Check the snapshot for output format.

I would have liked to indent the elements themselves to further distinguish them from the headings, but unfortunately I couldn't find a decent way to do that with the way debugDOM->prettyDOM->prettyFormat works. They inject their own newlines into the output, so if I try to indent it I can only indent the first line. I'd need to split the output of debugDOM up by lines and indent them individually... which seemed messy for a minor visual improvement.

I also purposefully left out the space between the delimiter bar ('-----') and the role name label. With the space it looks a lot louder visually (imo).

--------------------------------------------------
navigation:

<nav
    data-testid="a-nav"
/>

--------------------------------------------------
heading:

<h1
  data-testid="a-h1"
/>

vs

--------------------------------------------------

navigation:

<nav
    data-testid="a-nav"
/>

--------------------------------------------------

heading:

<h1
  data-testid="a-h1"
/>

[kentcdodds]

This looks awesome. Thank you!

[kentcdodds]

@all-contributors please add @michaellasky for code, tests, and docs

[allcontributors]

@kentcdodds

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

[kentcdodds]

🎉 This PR is included in version 5.3.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[kentcdodds]

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

[michaellasky]

Thanks Kent, you're making open source a lot of fun. I'll continue to watch and contribute.