Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-11-29] Console error cleanup. #51099

Closed
mallenexpensify opened this issue Oct 18, 2024 · 48 comments
Closed

[HOLD for payment 2024-11-29] Console error cleanup. #51099

mallenexpensify opened this issue Oct 18, 2024 · 48 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor Task

Comments

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Oct 18, 2024

Apologies for janky formatting below. The post in #expensify-open-source is easier to read

[x] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack

🧹 Some clean-up report on console errors / warnings as mentioned by @carlos here, quoting:

The idea is that we never have these errors and warnings, but we haven't been as diligent with this as should've been, so we've accumulated some warnings and errors, and we should fix them (I saw some warnings just now on another PR review).

The below errors / warnings were found on local dev, latest main w/ StrictMode enabled :downvote:
Note: Keep in mind that these were discovered during the main login flow, there are probably more if we dive deep into every flow (including Accounting integrations / Expensify Card, etc).

❗ Errors (Web)

  1. Warning: findDOMNode is deprecated in StrictMode. findDOMNode was passed an instance of Wrap which is inside StrictMode. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here: https://reactjs.org/link/strict-mode-find-node[](https://reactjs.org/link/strict-mode-find-node%60) - Origin file / line: react-dom.development.js:86
  2. Warning: Using UNSAFE_componentWillReceiveProps in strict mode is not recommended and may indicate bugs in your code. See https://reactjs.org/link/unsafe-component-lifecycles for details. - Origin file / line: react-dom.development.js:86

⚠️ Warnings (Web & Native)

  1. Animated: 'useNativeDriver' is not supported because the native animated module is missing. Falling back to JS-based animation. To resolve this, add 'RCTAnimation' module to this app, or remove 'useNativeDriver'. Make sure to run 'bundle exec pod install' first. Read more about autolinking: - Origin file / line: NativeAnimatedHelper.js:441
  2. accessibilityLabel is deprecated. Use aria-label. - Origin file / line: index.js:28
  3. "shadow*" style props are deprecated. Use "boxShadow". - Origin file / line: vendors-node_modules_react-navigation_stack_lib_module_navigators_createStackNavigator_js-nod-760534-30b975cdb07db961ae15.bundle.js:403
  4. props.pointerEvents is deprecated. Use style.pointerEvents - Origin file / line: index.js:28
  5. returnKeyType is deprecated. Use enterKeyHint. - Origin file / line: index.js:28

Android / iOS Native

  1. findHostinstance_DEPRECATED is deprecated in StrictMode. findHostinstance_DEPRECATED was passed an instance of AnimatedComponent(Textinput) which is inside StrictMode. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here: https://react.dev/link/strict-mode-find-node - Origin file / line: fabricUtils.ts:33
  2. Require cycle: src/CONST.ts -> src/libs/models/ BankAccount.ts -> src/CONST.ts Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. - Origin file / line: BankAccount.ts:32
  3. More Require cycle warnings in multiple files (48 instances).

Not sure what's the proto regarding who should fix and when should these be fixed but I guess there are 2 main options:
1️⃣ Somebody opens one PR where all of these are addressed & fixed.
2️⃣ Willing C+ which currently have opened PRs by contributors pick 1-2 errors / warnings to fix and merge with already opened PRs.
((3 votes for 1, none for 2))

cc @ikevin127 @cead22 @fabioh8010

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021847371950143140066
  • Upwork Job ID: 1847371950143140066
  • Last Price Increase: 2024-10-18
Issue OwnerCurrent Issue Owner: @kubabutkiewicz
@cead22
Copy link
Contributor

cead22 commented Oct 18, 2024

I say we make this an external task like we do for normal bugs

@fabioh8010
Copy link
Contributor

@cead22 Do you mind if we pass this to our expert agency like we discussed here?

I already passed context to @kubabutkiewicz to work on this, and we believe this task needs more careful planning and coordination to do all these fixings.

@cead22
Copy link
Contributor

cead22 commented Oct 18, 2024

That works for me

@mallenexpensify
Copy link
Contributor Author

@fabioh8010 , want me to assigne to @kubabutkiewicz? I'll make it a weekly too. I'm assuming we'll want C+ to review any PRs, comment if anyone disagrees.

@fabioh8010
Copy link
Contributor

Yes @mallenexpensify Please re-assign to @kubabutkiewicz ! Thanks

@mallenexpensify
Copy link
Contributor Author

@kubabutkiewicz can you comment please so I can assign? Thx

@kubabutkiewicz
Copy link
Contributor

Hello, Im Jakub from Callstack and would like to help with this issue

@mallenexpensify mallenexpensify added the External Added to denote the issue can be worked on by a contributor label Oct 18, 2024
Copy link

melvin-bot bot commented Oct 18, 2024

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

@melvin-bot melvin-bot bot changed the title Console error cleanup. [$250] Console error cleanup. Oct 18, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 18, 2024
Copy link

melvin-bot bot commented Oct 18, 2024

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

@mallenexpensify mallenexpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 18, 2024
@ikevin127
Copy link
Contributor

Tip

The warnings / errors listed by me in the Slack 🧵 are just the tip of the 🧊berg, the ones everyone can see as soon as the app is opened but there are many others that show up only when specific pages are opened (lots of lists with duplicated keys) and also many other that are platform specific, for example showing up on Android: Native or iOS: Native, some are Desktop specific (electron).

@mallenexpensify
Copy link
Contributor Author

@c3024 shared this too, which might be helpful/relevant

I reported 6 more warnings/errors more than a month ago here. GitHub issues were created for them and some of them were fixed. But, apparently the fixed issues returned again here and here. That is why they got accumulated. These likely need upstream fixes.

@melvin-bot melvin-bot bot added the Overdue label Oct 21, 2024
@kubabutkiewicz
Copy link
Contributor

Hey hey, I am coming with daily update for this issue.
Currently I am going through all errors/warning which appears on the app start and checking if this is something which we can fix in our codebase or its coming from third-party libraries, also checking in libs repos if there is already some effort to fix these issues or no.

Copy link

melvin-bot bot commented Oct 22, 2024

@eVoloshchak, @mallenexpensify, @kubabutkiewicz Whoops! This issue is 2 days overdue. Let's get this updated quick!

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Oct 22, 2024
@melvin-bot melvin-bot bot removed the Overdue label Oct 22, 2024
@mallenexpensify
Copy link
Contributor Author

Thanks for the update @kubabutkiewicz , I'm setting you as the issue owner and I updated the label to Weekly

@kubabutkiewicz
Copy link
Contributor

Hi I am providing an update and the plan which we want to follow.
As a first we want to tackle warnings/errors which are happening on the app startup.
I spend some time and listed warning/errors and possible root causes/fixes

All Platforms

Errors

Warning: TRenderEngineProvider: Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.

🚩 This error comes from react-native-render-html library.
Based on that issue, error is known but the repo seems to not be actively maintained at the moment. A solution could be to wait because there is an effort to move the project to another repository and then we can switch to it which should be easy. Another solution is apply the suggested patch in that issue, but I don't think adding another patch to our repo is good solution since its already hard to maintain these patches.

Web and Desktop

Warning: findDOMNode is deprecated and will be removed in the next major release. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here: https://reactjs.org/link/strict-mode-find-node

🚩 This most probably comes from react-native-web, currently react-native-web is using findDomNode under the hood here which will be removed in React 19 (more info here). There is already an issue in react-native-web repo and the maintainer is waiting for PRs which will fix this without breaking support for React 18. We can make this effort to create a PR there.

Warning: React does not recognize the dataElement prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase dataelement instead. If you accidentally passed it from a parent component, remove it from the DOM element.

🚩 This is coming from fullstrory/react-native library to be more precise it comes from here https://github.com/fullstorydev/fullstory-react-native/blob/4af1ab9c040cb647fb0f0afe25df42b986fd11de/src/index.ts#L165. It should be fixed on the library side so on web attributes won't be camelCase

Warning: React does not recognize the dataSourceFile prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase datasourcefile instead. If you accidentally passed it from a parent component, remove it from the DOM element.

🚩 This is coming from fullstrory/react-native library to be more precise it comes from here https://github.com/fullstorydev/fullstory-react-native/blob/4af1ab9c040cb647fb0f0afe25df42b986fd11de/src/index.ts#L175. It should be fixed on the library side so on web attributes won't be camelCase

⚠️ Warnings

Animated: useNativeDriver is not supported because the native animated module is missing. Falling back to JS-based animation. To resolve this, add RCTAnimation module to this app, or remove useNativeDriver. Make sure to run bundle exec pod install first. Read more about autolinking: https://github.com/react-native-community/cli/blob/master/docs/autolinking.md

✅ This is happening because there is not native driver on web there was an issue in react-native-web about that necolas/react-native-web#2455, I am suggesting to remove this prop when we are on web.

accessibilityLabel is deprecated. Use aria-label.

✅ 🚩 This is something which we can fix on our end on web platform we need to pass aria-label instead of accessibilityLabel

"shadow*" style props are deprecated. Use "boxShadow".

✅ 🚩 This is coming from react-native-web necolas/react-native-web#2620 but probably we need to fix it on our side we need somewhere we are using shadow* styles when its only iOS style

props.pointerEvents is deprecated. Use style.pointerEvents

✅ We can fix it on our side in web and desktop environment we can't pass pointerEvents as props we need to do that as styles

returnKeyType is deprecated. Use enterKeyHint.

✅ We can fix it on our side on web and desktop we need to pass enterKeyHint instead of returnKeyType

nativeID is deprecated. Use id.

✅ Same as above on web and desktop we need to pass id prop instead of nativeID

accessibilityRole is deprecated. Use role.

✅ 🚩 This is something which we can fix on our end. But its also present in third-party libraries like react-native-render-html

Desktop

Errors

⚠️ Warnings

Electron Security Warning (Insecure Content-Security-Policy) This renderer process has either no Content Security Policy set or a policy with "unsafe-eval" enabled. This exposes users of this app to unnecessary security risks.For more information and help, consult https://electronjs.org/docs/tutorial/security. This warning will not show up once the app is packaged.

✅ We can fix it on our side more info here https://www.electronjs.org/docs/latest/tutorial/security#7-define-a-content-security-policy

Native

Errors

Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?
Check the render method of ViewRenderer.

✅ we can fix it on our side We just need to add forwardRef in OptionRowRendererComponent

⚠️ Warnings

A lot of cycle imports so I will not list them all

✅ All those cycle imports are coming from our codebase. But probably fixing those will require major refactor how we import other libs.

No native ExponentConstants module found, are you sure the expo-constants's module is linked properly?

This is still pending investigation and looking for a root cause of that warning, but maybe someone from SWM could take a look since they are working closely with Expo.

Plan to tackle this issue (App startup)

First PR will fix:

  • Warning: Function components cannot be given refs.
  • Electron Security Warning (Insecure Content-Security-Policy) This renderer process has either no Content Security Policy set or a policy with "unsafe-eval" enabled. This exposes users of this app to unnecessary security risks.For more information and help, consult https://electronjs.org/docs/tutorial/security. This warning will not show up once the app is packaged.
  • Animated: useNativeDriver is not supported because the native animated module is missing. Falling back to JS-based animation. To resolve this, add RCTAnimation module to this app, or remove useNativeDriver. Make sure to run bundle exec pod install first. Read more about autolinking: https://github.com/react-native-community/cli/blob/master/docs/autolinking.md

Next PR will fix

  • accessibilityLabel is deprecated. Use aria-label.
  • "shadow*" style props are deprecated. Use "boxShadow".
  • props.pointerEvents is deprecated. Use style.pointerEvents
  • returnKeyType is deprecated. Use enterKeyHint.
  • nativeID is deprecated. Use id.
  • accessibilityRole is deprecated. Use role.

Next few PRs will be to fix import cycle errors each PR will be fixing 4/5 errors to avoid huge PRs and minimize risk of regressions as there are around 70+ import cycle errors
After that we will need to decide what we are doing with issues coming from external libraries, if we will work on them, wait for the maintainers, etc
Finally we will test different App flows in order to find other warnings/errors that can be triggered there
Next we will need to take a look on another flow, for example creating an expense to look for another warnings/errors

@cead22
Copy link
Contributor

cead22 commented Oct 29, 2024

This is a great breakdown, thank you!

@melvin-bot melvin-bot bot added the Weekly KSv2 label Nov 22, 2024
@kubabutkiewicz
Copy link
Contributor

Hi! Update on this:
We are still working on proposal for ulimatly fixing cycle imports.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 22, 2024
@melvin-bot melvin-bot bot changed the title Console error cleanup. [HOLD for payment 2024-11-29] Console error cleanup. Nov 22, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 22, 2024
Copy link

melvin-bot bot commented Nov 22, 2024

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

Copy link

melvin-bot bot commented Nov 22, 2024

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

If no regressions arise, payment will be issued on 2024-11-29. 🎊

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

@kubabutkiewicz
Copy link
Contributor

kubabutkiewicz commented Nov 25, 2024

Hi ! Update on this:
Continue work on ultimately fixing cycling dependencies in our repo. Currently testing different solutions which we came up with. Also I am waiting for review here

@dangrous
Copy link
Contributor

hey @kubabutkiewicz - is #52688 ready for my review? Melvin has been shirking on his duties so I'm not sure if I'm being correctly assigned where needed. Thank you!

@kubabutkiewicz
Copy link
Contributor

@dangrous yes review is needed but from C+ reviewer firstly

@kubabutkiewicz
Copy link
Contributor

Hey, update on that
We are still looking for a good and ultimate solution on fixing dependecy cycle.
Also I think I will start working on the next issue for cleaning the console which @mallenexpensify mentioned earlier here.

So we currently have fixed most of the console errors in which were mentioned in that issue

All Platforms

Errors

Warning: TRenderEngineProvider: Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.
This is agreed that we wait till libary maintainer fix that #52917 (comment)

Web and Desktop

Warning: findDOMNode is deprecated and will be removed in the next major release. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here: https://reactjs.org/link/strict-mode-find-node
Warning: React does not recognize the dataElement prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase dataelement instead. If you accidentally passed it from a parent component, remove it from the DOM element.
Warning: React does not recognize the dataSourceFile prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase datasourcefile instead. If you accidentally passed it from a parent component, remove it from the DOM element.

⚠️ Warnings

Animated: useNativeDriver is not supported because the native animated module is missing. Falling back to JS-based animation. To resolve this, add RCTAnimation module to this app, or remove useNativeDriver. Make sure to run bundle exec pod install first. Read more about autolinking: https://github.com/react-native-community/cli/blob/master/docs/autolinking.md
accessibilityLabel is deprecated. Use aria-label.

"shadow*" style props are deprecated. Use "boxShadow".
This is fixed on the library side but not implemented in our repo. What we will need to do is to upgrade react-navigation but we are currently on version 6 but the fix is merged on version 7. So I think we can leave it for now and wait until we will upgarde this lib as it will be a big effort and its better to do in seperate issue.
props.pointerEvents is deprecated. Use style.pointerEvents
returnKeyType is deprecated. Use enterKeyHint.
nativeID is deprecated. Use id.
accessibilityRole is deprecated. Use role.

Desktop

Errors

⚠️ Warnings

Electron Security Warning (Insecure Content-Security-Policy) This renderer process has either no Content Security Policy set or a policy with "unsafe-eval" enabled. This exposes users of this app to unnecessary security risks.For more information and help, consult https://electronjs.org/docs/tutorial/security. This warning will not show up once the app is packaged.

Native

Errors

Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?
Check the render method of ViewRenderer.

⚠️ Warnings

A lot of cycle imports so I will not list them all
This is still being worked on

No native ExponentConstants module found, are you sure the expo-constants's module is linked properly?
This is still pending investigation and looking for a root cause of that warning, but maybe someone from SWM could take a look since they are working closely with Expo.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 29, 2024
Copy link

melvin-bot bot commented Nov 29, 2024

Payment Summary

Upwork Job

BugZero Checklist (@mallenexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1847371950143140066/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@melvin-bot melvin-bot bot added the Overdue label Dec 2, 2024
Copy link

melvin-bot bot commented Dec 2, 2024

@dangrous, @eVoloshchak, @mallenexpensify, @kubabutkiewicz Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@mallenexpensify
Copy link
Contributor Author

Contributor+: @eVoloshchak due $250 via NewDot

start working on the next issue for cleaning the console which @mallenexpensify mentioned earlier #51099 (comment).

@kubabutkiewicz, sounds good to me, do you want to keep working off this GH or start a new one?

@melvin-bot melvin-bot bot removed the Overdue label Dec 2, 2024
@kubabutkiewicz
Copy link
Contributor

@mallenexpensify I think maybe it will be better to create new one specifically for cyclic dependencies issue as its a big one and it will be easier for keep tracking it

@mallenexpensify
Copy link
Contributor Author

@kubabutkiewicz new issue here. Please comment so I can assign to you.

@melvin-bot melvin-bot bot added the Overdue label Dec 5, 2024
Copy link

melvin-bot bot commented Dec 6, 2024

@dangrous, @eVoloshchak, @mallenexpensify, @kubabutkiewicz Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented Dec 10, 2024

@dangrous, @eVoloshchak, @mallenexpensify, @kubabutkiewicz Still overdue 6 days?! Let's take care of this!

@mallenexpensify
Copy link
Contributor Author

@kubabutkiewicz, can we close this?!?!

@melvin-bot melvin-bot bot removed the Overdue label Dec 10, 2024
@kubabutkiewicz
Copy link
Contributor

@mallenexpensify Yeah we can close it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor Task
Projects
None yet
Development

No branches or pull requests

8 participants