feat: debug options

[pierrezimmermannbam]

Summary

Adds optional filterProps option for debug util as discussed in #1135

Test plan

I added some tests using the option and filtering through propName, propValue or node
Additionally I checked that filtering props in debug doesn't mutate initial instance

[AugustinLF]

Looks pretty good overall (even though I'm not familiar with the way pretty-format works, two additional points:

• can we get mapNode also as an option?
• can you try adding this option to the global configure options? I believe there is a lot of values in setting a value for a whole project (I'll definitely filter out all my style props)

[pierrezimmermannbam]

@AugustinLF I added the option to configure the debugOptions globally and treated your other comments. Also I have updated the website docs with the new api. Regarding the mapNode option I was considering adding it in another pr if that's ok with you, so that the review process is shorter and also because I think this option by itself makes sense so the faster it is available the better

[mdjastrzebski]

I've done initial review on implementation and left some comments there. Overall I like the code so far, looks pretty solid.

The point I want to rise is the desired API we want to have.

The current API is:

export type FormatOptions = {
  filterProps?: (
    propName: string,
    propValue: unknown,
    node: ReactTestRendererJSON
  ) => boolean;
};

While it's quite flexible, I think we could investigate alternative approaches, based on the use cases we want to support.

Idea 1: allowlist/blocklist

As far as I understand the primary use case is to print only some relevant props and/or exclude some props that are cluttering the output. We are rarely interested in inspecting the prop value. We could achieve that by somewhat easier API:

type FormatOptions = {
  allowedProps?: string[];
  ignoredProps?: string[];
}

If specified allowedProps will contain the names props that will be printed, no other props would be printed by format. Otherwise, all props will be printed except the ones specified in ignoredProps.

Pros: simpler API for majority use case
Cons: less flexible

Idea 2: ability to transform props

Opposite to idea 1, this makes the API more flexible by allow prop transformation before printing. This could allow for both removal of props that are not interesting to as, or applying some transformation to their value that would improve the readability:

Version 1 - per prop

type FormatOptions = {
  mapProp?: (
    propName: string,
    propValue: unknown,
    node: ReactTestRendererJSON
  ) => [name, value];
}

Version 2 - per node

type FormatOptions = {
  mapProps?: (props: Record<string, unknown>) => Record<string, unknown>; // output props to be displayed
}

Examples

Original:

debug(<JSX />, { filterProps: (name, value, node) => name === 'style' });
debug(<JSX />, { filterProps: (name, value, node) => ['style', 'testID'].includes(name) });

Idea 1:

debug(<JSX />, { allowedProps: ['style'] });
debug(<JSX />, { allowedProps: ['style', 'testID'] });

Idea 2 - per prop:

debug(<JSX />, { mapProp: (name, value, node) => name === 'style' ? [name, value] : undefined });
debug(<JSX />, { mapProp: (name, value, node) => ['style', 'testID'].includes(name ? [name, value] : undefined });

Idea 2 - per node:

debug(<JSX />, { mapProps: ({ style }) => ({ style }) });
debug(<JSX />, { mapProps: ({ style, testID }) => ({ style, testID }) });

@pierrezimmermannbam could you provide typical examples you envision this option will handle so that we can figure out which API options is the best to address it.

[AugustinLF]

Looks good to me, my feedback has been addressed, I'll let you address @mdjastrzebski comments

[AugustinLF]

Good for me, I'll let you address @mdjastrzebski's comment meanwhile. I won't hit the Approve button to be sure it doesn't get merged by mistake :)

[pierrezimmermannbam]

@mdjastrzebski you're right we should consider alternative approaches. I plan to use this feature in the following ways :

• remove globally some props that aren't useful for debugging with configure api (style or path for instance)
• create custom debug utils that would be used for debugging each query using screen (for instance debugText would filter all props except children)
• use a more custom version in tests if the need arises but this should be pretty rare

Overall I'd mostly use this either with configure or have a set of utils that I would write only once so I like the idea of having a rather flexible api. However It should be accessible to users too so maybe we could document it a bit more with some use cases that we find relevant.

AllowedProps and ignoredProps is probably simpler than filterProps so I think it's interesting. I'm not sure however that it's that much simpler, plus i'm afraid that there may be some conflicts between ignored and allowed props especially if one has been set through configure. Also i was thinking of globally filtering props whose value is a function which you cannot do without access to the propValue. I also like to have a single option for filtering props, because we may add other options later
and the more there are the more it's going to be complicated to understand all of them.

mapProps is very interesting though, being able to flatten styles in the debug function for instance could be a very useful option. Are there any other use cases though ? Maybe directly having a flattenStyle option would be more convenient if that's not the case. What I'm afraid of is that it seems less obvious to me why you'd want to map props than why you'd want to filter props but it would indeed allow way more flexibility. I think I wouldn't mind having both the filterProps and the mapProps option though.

What do you think would be the best api based on the examples I provided ?

[mdjastrzebski]

@pierrezimmermannbam not sure if you're participating in Hacktoberfest but if so I've added hacktopberfest-accept, I plan to review this again in the following days.

[mdjastrzebski]

@pierrezimmermannbam

Imo based on your use cases I think mapProps (per node version) makes most sense and is also the most flexible approach. It allows for both filtering & transforming props (e.g. flatten styles), while also allowing for basic output on a prop combination, e.g. output style prop only if disabled props is not set, etc. I'm curious about @AugustinLF take on the API.

re accept list/ignore list priority: I think a reasonable assumption is that accept list takes priority over ignore list. If you have accept list then you do not use ignore list at all.

re ease of use: we should have some documentation examples for common use cases, that should clarify the usage.

re having both filterProp and mapProps that would make the API a bit confusing as we would have two functions doing very similar things, and also it's not clear how should they interact. Which should be used if both are present, etc

I would be wary of introducing niche options like flattenStyles as it can be done with mapProps and imo is not worth to complicating our API.

[AugustinLF]

I think it's fair to go with mapProps. It is an option for advanced users, so we should aim it to make it complete. And yes, for things like flattenStyles we could offer it as a recipe. It doesn't have to be in our API. Or if highly requested, we could expose it as an export, that could then be passed to the mapProps.

[pierrezimmermannbam]

I also agree, I'll change the api and add some examples of usage in the documentation

[pierrezimmermannbam]

@mdjastrzebski I changed the api to use mapProps instead and updated the website docs to include more examples of the option usage. I also treated the comments you left.

There is one last thing I'm not sure about though, should we keep the compatibility with previous versions for debugOptions ? The debug util is not meant to stay in the code, it's just a debugging tool so there should be little to no issues regarding the migration. I feel like it adds unnecessary complexity in the RNTL codebase while providing very little value to users. I think this could be merged as is and be included in a release before next major version but I would remove retro compatibility with the next major version, wdyt ?

[mdjastrzebski]

@pierrezimmermannbam This PR looks really solid. I've added some suggestions for finishing touches and edge cases.

Pls add some real-life examples to the docs on how users could you this to improve their debugging output.

[pierrezimmermannbam]

@mdjastrzebski I think I answered all your comments. I added an example to the doc and detailed more the flatten style example, do you think of anything else ?

[mdjastrzebski]

Looks great! Good work @pierrezimmermannbam 🚀

I've made some cosmetic tweaks to docs and code before merging.