[HOLD for payment 2024-11-29] Console error cleanup.
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)
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:86Warning: 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)
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:441accessibilityLabel is deprecated. Use aria-label.- Origin file / line: index.js:28"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:403props.pointerEvents is deprecated. Use style.pointerEvents- Origin file / line: index.js:28returnKeyType is deprecated. Use enterKeyHint.- Origin file / line: index.js:28
Android / iOS Native
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:33Require 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- More
Require cyclewarnings 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 Owner
Current Issue Owner: @kubabutkiewicz
I say we make this an external task like we do for normal bugs
@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.
That works for me
@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.
Yes @mallenexpensify Please re-assign to @kubabutkiewicz ! Thanks
@kubabutkiewicz can you comment please so I can assign? Thx
Hello, Im Jakub from Callstack and would like to help with this issue
Job added to Upwork: https://www.upwork.com/jobs/~021847371950143140066
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)
[!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).
@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.
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.
@eVoloshchak, @mallenexpensify, @kubabutkiewicz Whoops! This issue is 2 days overdue. Let's get this updated quick!
Thanks for the update @kubabutkiewicz , I'm setting you as the issue owner and I updated the label to Weekly
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
dataElementprop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercasedataelementinstead. 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
dataSourceFileprop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercasedatasourcefileinstead. 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:
useNativeDriveris not supported because the native animated module is missing. Falling back to JS-based animation. To resolve this, addRCTAnimationmodule to this app, or removeuseNativeDriver. Make sure to runbundle exec pod installfirst. 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:
useNativeDriveris not supported because the native animated module is missing. Falling back to JS-based animation. To resolve this, addRCTAnimationmodule to this app, or removeuseNativeDriver. Make sure to runbundle exec pod installfirst. 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
This is a great breakdown, thank you!
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 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
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!
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.mdits 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.
Hi! Update on this issue:
Triggered auto assignment to @dangrous, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
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
Hi! I just got assigned to this by Melvin. What's the action here? What can I help with? Thanks!
Good question @dangrous, I'm unsure why you were assigned Β―_(γ)_/Β―
Maybe becasue this PR needs attention from internal engineer π ?
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.
oh weird it assigned me to this issue but not actually to review the PR. Let me add myself and review it now!
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