Make sure expression before && is always boolean in React rendering in marketing page

[ecgan]

All Submissions:

• Have you followed the WooCommerce Contributing guideline?
• Does your code follow the WordPress' coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Closes #37175.

In this PR, we make sure that the expression before && is always boolean by using double negation !!.

In some cases where we are using a variable like shouldShowExtensions and isModalOpen which should already be boolean, we use !! in front of them anyway, so that we don't need to second guess it when we look at it, and we are future proofing it (e.g. when the variable type is changed from boolean to another type in the future).

How to test the changes in this Pull Request:

• Have you followed the Writing high-quality testing instructions guide?

Functionality test:

1. Make sure this fixes issue Make sure expression before && is always boolean in JSX #37175.
2. Test the marketing page (classic) and it works as expected.
3. Test the marketing page (beta multichannel) and it works as expected.

Code check:

1. Look into the marketing page code base and make sure all the expression before && in React rendering is always boolean.
   • The expression should contain !, !!, or a comparison operator like === or >=.

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?
• Have you included testing instructions?

FOR PR REVIEWER ONLY:

• I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

[ecgan]

@eason9487 , this fixes issue #37175 that you brought up during code review on my PR in #37044 (review). Could you help to review this please? 🙏

[github-actions]

Test Results Summary

Commit SHA: dd6856a

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 49s
E2E Tests	189	0	0	6	0	195	12m 51s

---

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

[codecov]

Codecov Report

Merging #37227 (dd6856a) into trunk (0cf5677) will decrease coverage by 0.0%.
The diff coverage is 32.0%.

Additional details and impacted files

@@            Coverage Diff             @@
##             trunk   #37227     +/-   ##
==========================================
- Coverage     46.7%    46.7%   -0.0%     
  Complexity   17191    17191             
==========================================
  Files          429      429             
  Lines        64845    64865     +20     
==========================================
+ Hits         30275    30284      +9     
- Misses       34570    34581     +11     

Impacted Files	Coverage Δ
...ugins/woocommerce/includes/class-wc-post-types.php	2.7% <0.0%> (-0.1%)	⬇️
...gins/woocommerce/includes/wc-product-functions.php	44.7% <100.0%> (+1.0%)	⬆️

... and 1 file with indirect coverage changes

[eason9487]

Thanks for fixing the render 0 issue.

The main issue mentioned in #37044 (review) has been fixed.

However, adding additional type casting might not be such a necessary precaution for some very deterministic and single types such as boolean or string.

In some cases where we are using a variable like shouldShowExtensions and isModalOpen which should already be boolean, we use !! in front of them anyway, so that we don't need to second guess it when we look at it, and we are future proofing it (e.g. when the variable type is changed from boolean to another type in the future).

The only falsy values that will be rendered are 0 and NaN, i.e., only the number type. When reading these !!, it will instead think one more time: Why it needs an additional type casting when no number type result will happen here?

Furthermore, assuming this was a good practice: "Always add a type casting to any type, for example, string, to prevent it from rendering a 0 in the future if it is changed to another type.", then another question it brings up would be: "Do all rendering statements like the following have to start with a precautionary check in front of them?"

woocommerce/plugins/woocommerce-admin/client/marketing/components/knowledge-base/index.js

Line 88 in dd6856a

<h3>{ post.title }</h3>

woocommerce/plugins/woocommerce-admin/client/marketing/components/knowledge-base/index.js

Line 91 in dd6856a

{ post.author_name }

woocommerce/plugins/woocommerce-admin/client/marketing/overview-multichannel/Campaigns/CreateNewCampaignModal/CreateNewCampaignModal.tsx

Line 87 in dd6856a

{ el.name }

woocommerce/plugins/woocommerce-admin/client/marketing/overview-multichannel/Campaigns/CreateNewCampaignModal/CreateNewCampaignModal.tsx

Line 90 in dd6856a

{ el.description }

woocommerce/plugins/woocommerce-admin/client/marketing/overview-multichannel/Channels/IssueStatus.tsx

Line 32 in dd6856a

{ registeredChannel.issueText }

There are very many such rendering statements. If these rendering statements are still considered not to require a precaution, then conversely, are those similar changes made in this PR still needed when there is clearly no problem now?

[ecgan]

@eason9487 , thanks for the review. 🙏

The only falsy values that will be rendered are 0 and NaN, i.e., only the number type. When reading these !!, it will instead think one more time: Why it needs an additional type casting when no number type result will happen here?

The short answer is that based on https://reactjs.org/docs/jsx-in-depth.html?#booleans-null-and-undefined-are-ignored:

make sure that the expression before && is always boolean

So I use !! to make sure the expression before && is always boolean, even when "no number type result will happen here".

Here's another thinking that I have. When I look at the issue, I did a search for && in the code base like the following:

By looking at the search result, with the !! in place, I can be certain that the issue does not happen. Without the !! in place, I have to do a bit more digging around. With isModalOpen, that is a bit more okay because it is strongly typed as boolean. With shouldShowExtensions variable which is of any type, it would require us to dig even deeper and execute the code to find out if it is really a boolean. So I chose to use !! consistently to save us the digging effort.

In the future, I'm hoping to get react/jsx-no-leaked-render in @wordpress/eslint-plugin or @woocommerce/eslint-plugin, so that we would be better protected against this issue happening.

Furthermore, assuming this was a good practice: "Always add a type casting to any type, for example, string, to prevent it from rendering a 0 in the future if it is changed to another type.", then another question it brings up would be: "Do all rendering statements like the following have to start with a precautionary check in front of them?"

But the examples you gave does not involve the use of &&, so it's not really related to our issue here...?

[eason9487]

The short answer is that based on https://reactjs.org/docs/jsx-in-depth.html?#booleans-null-and-undefined-are-ignored:

make sure that the expression before && is always boolean

Full description in the doc is:

To fix this, make sure that the expression before && is always boolean

I think the full description is telling how to fix the issue but doesn't mean to be a practice for all conditional rendering statements.

But the examples you gave does not involve the use of &&, so it's not really related to our issue here...?

I meant if it's important to prevent rendering 0 for the non-number type of statements in the future, then how does a dev determine whether those statements that don't even have a pre-check need the same prevention?

For example, consider the following code:

return (
  <div>
    Statement A:
    { el.description && (
      <FlexItem>
        { el.description }
      </FlexItem>
    ) }

    Statement B:
    { !! el.description && (
      <FlexItem>
        { el.description }
      </FlexItem>
    ) }

    Statement C:
    { el.description }
  </div>
);

Although TS has ensured that the el.description must be a string type, to prevent future changes of A from rendering an unexpected 0, it changed A to B. So, what should devs do for C now? If the number 0 appears in el.description in the future, then both A and C will render 0.

They have the same situation with the practice "to avoid rendering 0 when type is changed in the future", but why does only A need future-proofing and C doesn't?

Here's another thinking that I have. When I look at the issue, I did a search for && in the code base like the following:
[...]
By looking at the search result, with the !! in place, I can be certain that the issue does not happen. Without the !! in place, I have to do a bit more digging around. With isModalOpen, that is a bit more okay because it is strongly typed as boolean. With shouldShowExtensions variable which is of any type, it would require us to dig even deeper and execute the code to find out if it is really a boolean. So I chose to use !! consistently to save us the digging effort.

When reading code, the need to guess depends on the reader's expected familiarity with React, rather than whether a line of a statement looks like it 100% won't have this issue at all.

I could also say that when seeing shouldShowExtensions && or el.description &&, I can infer that their value ranges will not have 0 because I believe the devs deeply know the rendering of React and their code quality.

But, when seeing !! isModalOpen &&, I'm confused:

• "Is it possible that isModalOpen is not a boolean?"
• "I guess it could be so that it needs a type casting here."
• "I'm a bit worried if it would break other places when it's not actually boolean always."
• "Let's look around and search how it comes."
• "Oh ok, it's always a boolean value. Then, why it needs the redundant type casting?"
• Git blame the code, look up the file history, and finally find the reason in this PR:

  we use !! in front of them anyway, so that we don't need to second guess it when we look at it, and we are future proofing it (e.g. when the variable type is changed from boolean to another type in the future).

• "Well, it maybe saved me the second guess but it made me have already done much more survey and digging effort now."

This practice assures it won't have this PR's issue but it brings new questions to my mind, and then I still need to dig around. Overall it may not make for better readability or inferability.

In addition, by searching the codebase (5ed070d) with the following regex patterns, I don't really think this codebase needs this practice.

• ^\s+(\{ )?!! .+(?<!\.length)(?<!\.length \)) && \(?[\n\s]*<
  • 5 results
  • Only one still has the additional type casting when the type is definitely boolean.
• ^\s+(\{ )?(\( )?[\w\.\?\)]+ && \(?[\n\s]*<
  • 184 results

Perhaps what can be summarized are:

• The entire codebase has only one place for the explicit boolean variable to do an additional type casting.
• Overall, only 2.64% (5 / (5+184)) do the additional type casting.
• This issue rarely occurs in the render statements with a non-number variable, so the actual use cases of type casting for them are very rare. (Only 5)
• In conclusion, this practice is very rarely used and needed.

In the future, I'm hoping to get react/jsx-no-leaked-render in @wordpress/eslint-plugin or @woocommerce/eslint-plugin, so that we would be better protected against this issue happening.

IMO, this linting rule is mostly needed for repos powered by React Native, but it's verbose for this repo.

[eason9487]

While I don't recommend adding redundant type castings for conditional rendering statements that obviously won't render out an unexpected 0, the runtime results are still correct and I probably have no further thoughts on whether it's worth doing so for the same topic, therefore, I will be approving this PR to unblock the code review progress and leave the decision to the PR owner.

[ecgan]

@eason9487 , thanks so much for sharing your thoughts! 🙂 🙏

They have the same situation with the practice "to avoid rendering 0 when type is changed in the future", but why does only A need future-proofing and C doesn't?

To me, Statement C is expected to render whatever is given in el.description without condition. If it is changed to 0, It will render 0. So, rendering 0 here isn't unexpected.

But, when seeing ! isModalOpen &&, I'm confused: [...]

Just want to say that I especially appreciate this perspective, I didn't think of it this way, thanks for sharing this. 🙂

And thanks for approving this PR. I will proceed to merge it. Maybe as I work on this more, I will get to understand your point better and might come back to revert some of the code change here that I made. 😂

[ecgan]

But, when seeing ! isModalOpen &&, I'm confused: [...]

Just want to say that I especially appreciate this perspective, I didn't think of it this way, thanks for sharing this. 🙂

@eason9487 , I can open a PR to change the !! isModalOpen && to isModalOpen &&, and also for expressions that are already typed as boolean. For other expressions like shouldShowExtensions that is not typed as boolean, I'll maintain it as !! shouldShowExtensions &&. What do you think?

[eason9487]

To me, Statement C is expected to render whatever is given in el.description without condition. If it is changed to 0, It will render 0. So, rendering 0 here isn't unexpected.

I must clarify that if a variable that was always a string becomes possible to be a number, then this does not mean that the current implementation will be the expected result.

Whether or not rendering 0 is the expected result depends on when the need for change occurs rather than the present. To assert now that it must be expected or unexpected is a preconception, which is not a valid point. Otherwise, I could disprove this assertion right now with the same preconceived argument: if its value is changed to 0 and I think it should not be rendered, so rendering 0 is not expected.

But either one is a false statement. How it should be rendered depends on the demand when it occurs, not on the current one-sided assumptions.

Moreover, with the premise that el.description is always string, the difference between C and A B is simply that because A B need to control the layout, not because they need to control whether to render the string.

Even if C has no explicit condition, it still has an implicit rendering condition and is identical to A B, i.e., the empty string is not rendered (no content to render). This can not be used to infer that "C should render whatever the value is given.", especially the 0 is not in the current value range.

Otherwise, if the assertion was true that "it's expected to render whatever is given", then you probably also meant that rendering out a NaN is also expected.

[eason9487]

I can open a PR to change the !! isModalOpen && to isModalOpen &&, and also for expressions that are already typed as boolean.

It would be nice and appreciated.

For other expressions like shouldShowExtensions that is not typed as boolean, I'll maintain it as !! shouldShowExtensions &&. What do you think?

I would still suggest removing unnecessary !! added in this PR.

If the type of a variable can be narrowed down to a very specific type, it is better practice to ensure this at the beginning, because all the statements associated with it will become easier to code and better to read.

In the case of shouldShowExtensions, if it is not typed as boolean but can be changed to definitely boolean, then I would choose to do this:

// Change this
const shouldShowExtensions =
	getAdminSetting( 'allowMarketplaceSuggestions', false ) &&
	currentUserCan( 'install_plugins' );

// to
const shouldShowExtensions =
	Boolean( getAdminSetting( 'allowMarketplaceSuggestions', false ) ) &&
	currentUserCan( 'install_plugins' );

[ecgan]

In the case of shouldShowExtensions, if it is not typed as boolean but can be changed to definitely boolean, then I would choose to do this: [...]

Ahh of course! I should have thought of that! 😂 Okay, I can work with that.

By the way, you were using Boolean() in your code example. Is there any preference or reason to use Boolean over !!? I'm fine with either one, just thinking that !! would make the code shorter. If we should use Boolean, then I can change them all in the same PR.

// Using Boolean:
const shouldShowExtensions =
	Boolean( getAdminSetting( 'allowMarketplaceSuggestions', false ) ) &&
	currentUserCan( 'install_plugins' );

// Using !!:
const shouldShowExtensions =
	!! ( getAdminSetting( 'allowMarketplaceSuggestions', false ) ) &&
	currentUserCan( 'install_plugins' );

[eason9487]

By the way, you were using Boolean() in your code example. Is there any preference or reason to use Boolean over !!? I'm fine with either one, just thinking that !! would make the code shorter. If we should use Boolean, then I can change them all in the same PR.

This is just my coding preference, the main reason is that compared to !! ... , Boolean( ... ) is clear in terms of vision, intent, and operator precedence and scope. But in terms of code review or coding conventions, both are good to me. I guess it doesn't need to open another PR to replace !! with Boolean().

[ecgan]

I can open a PR to change the !! isModalOpen && to isModalOpen &&, and also for expressions that are already typed as boolean.

It would be nice and appreciated.

In the case of shouldShowExtensions, if it is not typed as boolean but can be changed to definitely boolean, then I would choose to do this: [...]

Ahh of course! I should have thought of that! 😂 Okay, I can work with that.

@eason9487 , I have created PR #37452 for the above. Let me know what you think. 🙏