[HOLD for payment 2022-10-24] [$250] Resolve cyclic dependencies lint errors

[MonilBhavsar]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

There are some silenced cyclic dependency lint errors in our codebase. They were silenced in this PR as we didn't want to block our project just because of them.
Cyclic dependency doesn't break anything but it's good to optimize code and get rid of them.

Action Performed:

Break down in numbered steps

1. Uncomment // eslint-disable-next-line import/no-cycle lines in codebase
2. Run npm run lint

Expected Result:

No lint errors should show

Actual Result:

Cyclic dependency lint error appears

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Users are not affected

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number:
Reproducible in staging?:
Reproducible in production?:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Upwork job URL: https://www.upwork.com/jobs/~0105a5aec0a694b518
Issue reported by:
Slack conversation:

View all open jobs on GitHub

[roryabraham]

Should we make this external?

[roryabraham]

If it's internal and doesn't have an assignee I worry it won't get done quickly

[MonilBhavsar]

I made it internal as it was introduced by refactoring PR. But agree, we can make it external to get is resolved quickly

[melvin-bot]

Triggered auto assignment to @michaelhaxhiu (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 - @thesahindia (External)

[melvin-bot]

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

[aimane-chnaif]

Proposal

RCA:
Require cycle: src/libs/DateUtils.js -> src/libs/actions/PersonalDetails.js -> src/libs/deprecatedAPI.js -> src/libs/Middleware/index.js -> src/libs/Middleware/Reauthentication.js -> src/libs/Authentication.js -> src/libs/actions/SignInRedirect.js -> src/libs/DateUtils.js

Solution:
DateUtils -> PersonalDetails
So we can remove PersonalDetails import in DateUtils, which is used only in updateTimezone() function.
DateUtils.updateTimezone() function is not used anywhere in the app
But for future use, we can move updateTimezone() from DateUtils to PersonalDetails instead of removal.
Please check detailed code proposal here: aimane-chnaif@f120ae9

[thesahindia]

@aimane-chnaif's proposal looks good to me.

C+ reviewed 🎀👀🎀

cc: @stitesExpensify

[michaelhaxhiu]

Just one question @aimane-chnaif - why are we saying RCA? Are we suggesting this is a regression (i.e. it used to work properly, but now it doesn't)?

[aimane-chnaif]

Just one question @aimane-chnaif - why are we saying RCA? Are we suggesting this is a regression (i.e. it used to work properly, but now it doesn't)?

@michaelhaxhiu I just explained detail about those lint errors. you can ignore that if RCA means only a regression keyword

[parasharrajat]

Prefer to use Root cause for explaining the cause. And RCA or Regression when you want to refer to regression.

[aimane-chnaif]

ah ok I thought RCA meant Root Cause Analysis, didn't know that R is Regression

[michaelhaxhiu]

Haha sorry if that was confusing @aimane-chnaif you haven't done anything wrong here! 💙

This is totally semantics at this point, but let me break it down for clarity. And let me know if you think any external documentation has made this confusing for you as a contributor - I'll gladly revise it, if so:

• We perform a Root Cause Analysis (RCA) when there is a regression.
• We use the term regression to say something used to work, but then broke and stopped working.
• We do a RCA to identify the root cause of the regression itself and ultimately prevent it from happening again. So we don't get repeated bugs in the same area of the code.
• If it's a new bug, then we don't call it a regression. So new bugs don't require an RCA.

[michaelhaxhiu]

Though it's a new bug and not a regression, I think it's better to explain why this happens, rather than proposing only a solution.

Totally agree! As you continue to work on jobs, feel free to identify the "root cause" in your Problem statement like @parasharrajat mentioned :) love it.

[michaelhaxhiu]

@stitesExpensify can you confirm if this proposal is good to proceed, please?

#11102 (comment)

[roryabraham]

Stepping in to help cover @stitesExpensify while he's OOO

[melvin-bot]

📣 @aimane-chnaif You have been assigned to this job by @roryabraham!
Please apply to this job in Upwork 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 📖

[michaelhaxhiu]

Upwork job - https://www.upwork.com/jobs/~0105a5aec0a694b518

[melvin-bot]

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

• resolve cyclic dependencies lint errors #11782

If no regressions arise, payment will be issued on 2022-10-24. 🎊

[michaelhaxhiu]

Invited @thesahindia to the job, so we can pay both of you at the same time. @thesahindia can you confirm when you accepted it?

[michaelhaxhiu]

Will get this paid today

[thesahindia]

Applied 🚀

[michaelhaxhiu]

All paid.