search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.52k stars 2.88k forks source link

[HOLD for payment 2023-11-22] [$500] Get rid of conditional hook render #29527

Closed luacmartins closed 11 months ago

luacmartins commented 1 year ago

Problem

We introduced a conditional hook render here, which is against React hook rules and is an anti-pattern

Why is this important

Fixes an anti-pattern

Solution

Refactor the code to avoid the conditional hook call

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0178326c7786db6630
  • Upwork Job ID: 1720488262428413952
  • Last Price Increase: 2023-11-03
  • Automatic offers:
    • dukenv0307 | Contributor | 27547818
luacmartins commented 1 year ago

Not a priority

luacmartins commented 1 year ago

Not a priority

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0178326c7786db6630

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)

dukenv0307 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

We introduced a conditional hook render https://github.com/Expensify/App/pull/29525, which is against React hook rules and is an anti-pattern

What is the root cause of that problem?

We're using this hook conditionally. We need to do that because if we're not inside a tab navigator, calling useTabAnimation will throw an error here. The design of the hook (that we added) is not ideal, normally we should never throw an error inside the hook, we just return values according to what's expected by us. If we're not in the tab navigator, we can simply return the animation value as is (undefined).

What changes do you think we should make in order to solve the problem?

In the useTabAnimation hook, if we're not in the tab navigator, we can simply return the animation value as is (undefined), without throwing an error. This is best practice when designing a hook, if it's not in an environment where it can operate properly, the hook should simply return a value that indicates such (in this case is undefined), precisely to avoid situations of conditional hook rendering like this.

Or additionally we can add a param like isInTabNavigator to the hook, if it's false, the hook will simply return undefined. This will make it clear that the hook is expected to be able to handle cases where it's not in the tab navigator, in which case it becomes a noop.

What alternative solutions did you explore? (Optional)

We can introduce a HOC withTabAnimation, which will, depends on whether isInTabNavigator is true, will return a WrappedCompoent with/or without the useTabAnimation logic and the tabAnimation prop.

Pseudo-code:

WrappedComponentWithTabAnimation: 
      const animation = useTabAnimation();

      return <WrappedComponent
          tabAnimation={animation}
      >

withTabAnimation:
      if (isInTabNavigator) {
           return WrappedComponentWithTabAnimation
      }

      return WrappedComponent;

Then we can use withTabAnimation anywhere a conditional tabNavigation is needed

parasharrajat commented 1 year ago

@dukenv0307 This is indeed a possible solution but not the best solution. Just like other navigation hooks, we should force the user to make the decision that they only use these hooks under a valid context. Thus it will save us from unexpected bugs. So we should find a way to use this hook where the tab navigator is applicable. if you look at the old code, it was trying to do the same in this https://github.com/Expensify/App/pull/29525.

This is best practice when designing a hook.

Do you have supporting reference for this statement?

erquhart commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

A hook is being conditionally rendered, which breaks the rules of hooks.

What is the root cause of that problem?

The hook will throw an error if not inside of a Tab Navigator context, so conditional rendering was used as a workaround.

What changes do you think we should make in order to solve the problem?

Split out the body of NavigationAwareCamera component in NavigationAwareCamera.native.js to two separate components, NavigationAwareCameraWithTab and NavigationAwareCameraWithoutTab (names can be changed if preferred, these are just suggestions).

Here's how this pattern makes a difference, in pseudo code:

Before:

const Component = () => {
  // illegally rendering hook conditionally
  const animation = someCondition ? useHook() : undefined

  // ...body of component
}

After:

// Body of component that always render hook
const ComponentWithHook = () => {...}

// Body of component, never renders hook
const ComponentWithoutHook = () => {...}

const Component = () => {
  // conditionally rendering components is okay
  if (someCondition) {
    return <ComponentWithHook/>
  }
  return <ComponentWithoutHook/>
}

This approach is good for keeping implementation details within the component - the consumer doesn't need to think about whether it is inside of a tab navigator or not, the component handles this automatically.

What alternative solutions did you explore? (Optional)

The previously posted solution doesn't account for the useTabAnimation() hook throwing an error if rendered outside of tab context. I believe the mention in that solution of "best practice when designing a hook" is meant to reference that hooks should ideally be designed to run without this kind of throwing behavior so they're more versatile, but this hook isn't in the control of Expensify, it's being patch-exported from react-navigation internals.

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

dukenv0307 commented 1 year ago

@parasharrajat I've added an alternate solution that will create a new HOC that is reusable for any component needing conditional tabAnimation moving forward, pls kindly take a look, thanks!

dukenv0307 commented 1 year ago

Do you have supporting reference for this statement?

@parasharrajat kindly check this article. Particularly:

A hook that throws an error while being executed without handling it internally (unhandled) is not done properly.

This hook is very dangerous as it’ll crash the whole app when executed. The reason is that it can throw an exception that cannot be handled. It is impossible to use a try/catch inside a React component as it’ll break the rules of hooks.

It's the same as throwing an app-crashing exception and at the same time discourages people from handling it elsewhere

parasharrajat commented 1 year ago

@erquhart your proposal didn't explain how it will prevent the issue mentioned at line https://github.com/Expensify/App/pull/29525/files#diff-a26743da98663c8a37d6666dd21700ae7c687d9564e6a79e0ab077c78cdf151aR28

erquhart commented 1 year ago

@parasharrajat it's in the 3 bullet list - will conditionally render either a subcomponent with or without the hook based on isTabNavigator prop, so it's no longer conditionally rendering the hook. I can add pseudo code.

erquhart commented 1 year ago

@parasharrajat updated with pseudo code to make the difference more clear.

luacmartins commented 1 year ago

still reviewing proposals

parasharrajat commented 1 year ago

I will have the update in few minutes.

parasharrajat commented 1 year ago

HOC looks handy as Tabs can be used at many places in the App. But both proposals suggest the same thing. So @luacmartins can decide the final approach.

@dukenv0307 proposal looks for more DRY.

:ribbon: :eyes: :ribbon: C+ reviewed

melvin-bot[bot] commented 1 year ago

Current assignees @puneetlath and @luacmartins are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

luacmartins commented 1 year ago

I agree that an HOC seems the way to go here.

melvin-bot[bot] commented 1 year ago

📣 @parasharrajat Please request via NewDot manual requests for the Reviewer role ($500)

melvin-bot[bot] commented 1 year ago

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

dukenv0307 commented 1 year ago

@parasharrajat The PR is ready for review.

melvin-bot[bot] commented 11 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 11 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.99-0 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-11-22. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 11 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

puneetlath commented 11 months ago

@parasharrajat friendly reminder about the checklist.

parasharrajat commented 11 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

puneetlath commented 11 months ago

Payment summary:

Thanks everyone!

parasharrajat commented 11 months ago

Payment requested as per https://github.com/Expensify/App/issues/29527#issuecomment-1824658419

JmillsExpensify commented 11 months ago

$500 payment approved for @parasharrajat based on comment above.