App icon indicating copy to clipboard operation
App copied to clipboard

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

Open mallenexpensify opened this issue 1 year ago β€’ 47 comments

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

:broom: 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).

:exclamation: 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

:warning: 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: :one: Somebody opens one PR where all of these are addressed & fixed. :two: 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

mallenexpensify avatar Oct 18 '24 17:10 mallenexpensify

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

cead22 avatar Oct 18 '24 17:10 cead22

@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.

fabioh8010 avatar Oct 18 '24 17:10 fabioh8010

That works for me

cead22 avatar Oct 18 '24 17:10 cead22

@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.

mallenexpensify avatar Oct 18 '24 18:10 mallenexpensify

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

fabioh8010 avatar Oct 18 '24 18:10 fabioh8010

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

mallenexpensify avatar Oct 18 '24 18:10 mallenexpensify

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

kubabutkiewicz avatar Oct 18 '24 19:10 kubabutkiewicz

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

melvin-bot[bot] avatar Oct 18 '24 20:10 melvin-bot[bot]

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

melvin-bot[bot] avatar Oct 18 '24 20:10 melvin-bot[bot]

[!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).

ikevin127 avatar Oct 18 '24 20:10 ikevin127

@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.

mallenexpensify avatar Oct 21 '24 17:10 mallenexpensify

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.

kubabutkiewicz avatar Oct 22 '24 14:10 kubabutkiewicz

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

melvin-bot[bot] avatar Oct 22 '24 18:10 melvin-bot[bot]

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

mallenexpensify avatar Oct 22 '24 21:10 mallenexpensify

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 https://github.com/necolas/react-native-web/issues/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 https://github.com/necolas/react-native-web/issues/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

kubabutkiewicz avatar Oct 29 '24 09:10 kubabutkiewicz

This is a great breakdown, thank you!

cead22 avatar Oct 29 '24 14:10 cead22

Jumping the gun here but wanted to check before I forget.... @kubabutkiewicz after this issue is completed, do you think it makes sense to review other "console error" GHs in E/App to see about batching some together and fixing those? I see 11 open issues now that might be console error ones.

mallenexpensify avatar Oct 30 '24 18:10 mallenexpensify

@mallenexpensify yes of course, currently we are focusing on console errors which are happening on the app start because we wanted to work one by one flow in the next phase we can work on different flows

kubabutkiewicz avatar Oct 31 '24 14:10 kubabutkiewicz

Hi ! I created first PR with few fixes for console errors and warning. Its tackling 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
  • 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.
  • 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.

Also

  • returnKeyType is deprecated. Use enterKeyHint.
  • nativeID is deprecated. Use id.
  • accessibilityRole is deprecated. Use role.
  • accessibilityLabel is deprecated. Use aria-label.

Were fixed by upgrading React-native-web lib within effort for other issue

So whats next: @JKobrynski is helping me to resolve eslint warning for cycle imports I will be working on "shadow*" style props are deprecated. Use "boxShadow". This issue needs to be addressed with a fix in the react-navigation repository. Additionally, the warning regarding props.pointerEvents being deprecated is originating from the react-native-web library. However, the root cause of this warning is a problem within the react-navigation code. I've spoken with Satya, a maintainer of react-navigation, and they shared that they are aware of this issue but it is not a straightforward problem to resolve, and it is not currently a critical priority. Given the complexity involved in fixing this warning and the maintainer's assessment that it is not a critical issue, we believe it may be best to hold off on addressing it for now and instead wait for the react-navigation team to tackle it as part of their future work. Please let me know what you think!

kubabutkiewicz avatar Nov 07 '24 16:11 kubabutkiewicz

Hello! I am leaving an update whats going on.

  • I created a PR with fixing few warnings/errors in our codebase, currently waiting for C+ review.
  • Created a PR in react-native-keyboard-controller to fix 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 its already merged.
  • Created a PR in react-navigation repo. Waiting for a review.
  • Right now me and @JKobrynski who is helping me are working on cycle imports warnings in the metro dev console since there are 100+ of them. I am also checking why those warnings are only there and why linter dont catch them.

kubabutkiewicz avatar Nov 14 '24 13:11 kubabutkiewicz

Hi! Here is an update from me

  • Managed to fix plenty of require cycles
  • Created a PR with the fixes

JKobrynski avatar Nov 15 '24 15:11 JKobrynski

Hi! Update on this issue:

  • PR in react-navigation still waiting for review
  • PR in our codebase for fixing few warnings/errors also waiting for review
  • Working on the PR for fixing cycle imports

kubabutkiewicz avatar Nov 16 '24 10:11 kubabutkiewicz

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

melvin-bot[bot] avatar Nov 17 '24 20:11 melvin-bot[bot]

Hey! I am leaving update on that:

  • PR to react-navigation got merged ! now we will need to upgrade react-navigation in our project
  • Still working on cycle dependecies warning, working on some solution which could be implemented to fix them without huge refactors
  • Also prepering next PR with batch of fixes for few warnings

kubabutkiewicz avatar Nov 18 '24 20:11 kubabutkiewicz

Hi! I just got assigned to this by Melvin. What's the action here? What can I help with? Thanks!

dangrous avatar Nov 18 '24 21:11 dangrous

Good question @dangrous, I'm unsure why you were assigned Β―_(ツ)_/Β―

mallenexpensify avatar Nov 19 '24 00:11 mallenexpensify

Maybe becasue this PR needs attention from internal engineer πŸ˜„ ?

kubabutkiewicz avatar Nov 19 '24 07:11 kubabutkiewicz

Update:

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.

and

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.

seem to have been fixed, probably by rn-web fix that was applied. We checked both web and desktop on the latest main and they don't show up anymore.

JKobrynski avatar Nov 19 '24 09:11 JKobrynski

oh weird it assigned me to this issue but not actually to review the PR. Let me add myself and review it now!

dangrous avatar Nov 19 '24 17:11 dangrous

Hi! update from me I am working right now on ultimate solution for cycle imports. Currently working on a P/S and POC with benchamarks etc. to make a change how we are importing modules in the app to not create cycle imports

kubabutkiewicz avatar Nov 19 '24 20:11 kubabutkiewicz