Migrate index.js to function component

[marcaaron]

Class Component Migration

Filenames

• src/components/Hoverable/index.js
  • type: React.Component
  • has state values: true
  • has refs: true
  • has context: false
  • lifecycle methods: componentDidMount,componentWillUnmount

Task

• We currently have some class components in our codebase that we would like to refactor to a function component.
• Here's a link with some general advice on how to refactor a class component to a function component: https://react.dev/reference/react/Component#alternatives
• If you need additional guidance, please ask in #expensify-open-source
• Test for any regressions and verify that there are no breaking changes

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019c6ea0a458401e24
• Upwork Job ID: 1701785419929559040
• Last Price Increase: 2023-10-04

[muxriddinmuqimov77]

I'd like to work on this issue

[s-alves10]

I'd love to work on this.

[vdem0115]

I can work on this issue.

[olexyt]

I can work on this issue.

[rayane-djouah]

Can I work on this issue?

[mkhutornyi]

I'd like to work on this.

[aditygrg2]

Hello, I would like to work on this issue.

[kacper-mikolajczak]

Hi, I am Kacper from Callstack and I’d like to work on this issue :)

[melvin-bot]

📣 @kacper-mikolajczak! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019c6ea0a458401e24

[melvin-bot]

Triggered auto assignment to @peterdbarkerUK (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[blaw023]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~0167e44d60f727e86a

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[blaw023]

Proposal

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

Migrate src/components/Hoverable/index.js into a functional component.

What is the root cause of that problem?

new feature

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

We need to migrate index.js into a functional component. We can do this by using the following hooks:

• useEffect
• useRef
• useState

[melvin-bot]

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

[melvin-bot]

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

• Refactor Hoverable component into functional #29052

If no regressions arise, payment will be issued on 2023-10-25. 🎊

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.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @ArekChr does not require payment (Contractor)
• @kacper-mikolajczak does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[youssef-lr]

@kacper-mikolajczak I'm seeing this in production when I resize to mobile view from the console, could it be related to this PR?

[youssef-lr]

I can confirm that reverting the PR locally fixes it for me

Screen.Recording.2023-10-19.at.19.14.51.mov

[kacper-mikolajczak]

Hi @youssef-lr, it looks like the error is caused by the BoundsObserver + Hoverable combo, which was investigated before and seemed to be fixed. Not for long 😄 Is it critical enough that we want to revert the PR? Wonder if there are more errors, as the original issue is gone.

I will look into it, thanks for the feedback!

[youssef-lr]

Nobody has caught this in staging nor prod yet, so I guess it's not easy to reproduce? Weirdly, when I resize the window manually the error doesn't seem to happen. But pressing CMD+Option+J to reveal dev tools which is already set to mobile view triggers it. So I'm not totally sure about how critical it is..we can wait and find out if it's occurring for more users.

[kacper-mikolajczak]

Yeah, it stopped crashing "in the app" but still happens with scenario you brought up - interesting 🤔 My first thoughts are:

• race condition, but that would mean we should have the crash occasionally happening in the original scenario as well and not always when opening the devtools. On the other hand, there is possibility that some listener in BoundsObserver is not being triggered when window changes that fast
• BoundsObserver + Hoverable seem to have issues with passing refs properly

Hopefully it will be resolved sooner than later 🤞

[kacper-mikolajczak]

@cubuspl42 you might be interested in this 👀 🕵️

[cubuspl42]

If there's any problem with bounds-observer, you can create an issue, I might be able to fix it if there's a bug there!

I'm actually surprised nothing turned up for so long, at least nothing I'm aware of.

[cubuspl42]

Known limitations are listed here

[kacper-mikolajczak]

Thank you for the feedback! I will try to double check what is causing the issue and if the root cause is indeed on the library side, I will create the issue and hopefully resolve it as well 🤞

[kacper-mikolajczak]

@youssef-lr Here is the continuation of an issue described in #29844 (comment)

[kacper-mikolajczak]

@youssef-lr I've tried to investigate what is causing the app to crash when responsive view is triggered via devtools. I could not really find a proper reason - it looks like it only happens when we open the devtools for the first time:

• no error when rapidly changing window size
• no error when going between many screen sizes in devtools' responsive view
• no error when app is being opened in devtools's responsive view
• window size does not matter, with larger/smaller screens it's still triggering

It seems that some refs are not being properly attached, or at least not in time for BoundsObserver to be ready, thus BoundsObserver triggers its error based on missing ref.

I tried to prepare reproducible sample with BoundsObserver but it did not give any meaningful results - the app just works.

For today I have no further ideas how to solve the issue. Sorry! I will try to find a way soon!

[youssef-lr]

Thank you so much for digging into this @kacper-mikolajczak, appreciate it!

[kacper-mikolajczak]

@youssef-lr Here is a solution that should solve the issue here as well: #30278 (comment)

PR is under review 👍

[peterdbarkerUK]

Deployed to staging

[peterdbarkerUK]

With this now on prod, @youssef-lr can you confirm this is working as expected (and if so, close out the issue)?

[youssef-lr]

Working great @peterdbarkerUK!

[Beamanator]

Woohoo ! That means we can finally close this out? :D

[youssef-lr]

Yup!