Add alias to hidden in query options

[MattAgn]

This PR aims to tackle this issue. Basically the idea is to add a new option includeHidden that will be an alias to the query option hidden but much clearer. We keep the old name hidden for compatibility with RTL

Summary

• Alias hidden query option to hidden
• Alias defaultHidden config to defaultIncludeHidden
• add new alias to docs

Test plan

• Check new includeHidden option is working for all queries
• Check new defaultIncludeHidden option is working

Remaining work

• Choose a naming between includeHidden and includeHiddenFromAccessibility

Resolves #1194

[MattAgn]

@mdjastrzebski I've got some doubts about my jsdocs for the hidden alias, is it clear?
I was also wondering how to deal with the fact that we have two options (hidden and includeHidden) with the same meaning. Ideally, I guess we'd want to have both options in the api but internally to handle only one. However, it seems much more simple to handle the reconciliation in the findAll (both for the global config and the queries). I guess as long as we handle the hidden functionality only in findAll it's fine but if someone were to add a new feature elsewhere, there is a risk they might not handle both options properly.
What are your thoughts on that?

[codecov]

Codecov Report

Base: 94.49% // Head: 94.53% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (fa35bc9) compared to base (a178c55).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head fa35bc9 differs from pull request most recent head b891395. Consider uploading reports for the commit b891395 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1220      +/-   ##
==========================================
+ Coverage   94.49%   94.53%   +0.03%     
==========================================
  Files          42       42              
  Lines        2927     2946      +19     
  Branches      435      438       +3     
==========================================
+ Hits         2766     2785      +19     
  Misses        161      161              

Impacted Files	Coverage Δ
src/config.ts	100.00% <100.00%> (ø)
src/helpers/findAll.ts	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mdjastrzebski]

const includeHidden = 
  options?.includeHidden ??
  options?.hidden ??
  getConfig()?.defaultIncludeHidden ??
  getConfig()?.defaultHidden
 }

[MattAgn]

i wanted to avoid nested operators to maximise readibility, do you find it more readable your way?

[mdjastrzebski]

Yes, I find it much more readable, as a single nullish coalescing operator. Having it as separate intermediate vars would be useful if it used a mix of two operators (like && and ||) with different precedence, etc. In case all vars use the same operator, I found putting it together removes the question why are there some intermediate variables :-)

[MattAgn]

@mdjastrzebski I pushed a new commit with the other naming includeHiddenFromAccessibility so we can compare
Which do you prefer?
on my end, I must admit includeHiddenFromAccessibility is verbose, especially defaultIncludeHiddenFromAccessibility and feels a bit too much... but includeHidden still seems a bit unclear to me (I mean to us it's clear since we've been talking about it for weeks but don't know about the users). Maybe a good compromise would be includeHiddenElements? I'll push a new commit with that naming to see how it feels).

[mdjastrzebski]

@MattAgn includeHiddenElements seems like a reasonable option. I would give it a go an see how the resulting code looks.

[mdjastrzebski]

style: in our tests (also in other test files) I think it would be enough to use recommended includeHiddenElements option name and avoid polluting them with hidden all the time. We might want to have some few separate tests that would check that hidden alias is also used.

[mdjastrzebski]

Or in case you find it useful the current way, just put the includeHiddenElements first as the recommended option.

[MattAgn]

extracting them in another test file sounds good :) (helped me find a bug in the process)

[mdjastrzebski]

I like the new name includeHiddenElements seems to strike a proper balance between being readable and concise. As they say "naming is hard" but I think you nailed it 👍🏻

Regarding usage I would make sure we emphasis the readable variant, i.e. put it first in relevant tests, examples, docs, etc.

At this stage the goal for hidden alias is to help folks migrating from RTL or building dual RNTL/RTL wrappers. So it should work but should not be prominent at all not to confuse the main audience.

[MattAgn]

@mdjastrzebski thanks for the review, should be all good now :)

I've fixed a bug I introduced as well:

• since we have default values for defaultIncludeHiddenElements: true,
• configure({ defaultHidden: false }) would get us a config with { defaultHidden: false, defaultIncludeHiddenElements: true }
• in findAll, we would then have includeHiddenElements = true since getConfig().defaultIncludeHiddenElements would not be undefined

That's why I added some code in the configure option to tackle that

[mdjastrzebski]

👍🏻