-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Comments
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 ( |
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
|
Hey hey, I am coming with daily update for this issue. |
@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 |
Hi I am providing an update and the plan which we want to follow. All Platforms❌ Errors
🚩 This error comes from react-native-render-html library. Web and Desktop
🚩 This most probably comes from react-native-web, currently react-native-web is using
🚩 This is coming from
🚩 This is coming from
✅ 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.
✅ 🚩 This is something which we can fix on our end on web platform we need to pass aria-label instead of accessibilityLabel
✅ 🚩 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
✅ 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
✅ We can fix it on our side on web and desktop we need to pass
✅ Same as above on web and desktop we need to pass
✅ 🚩 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
✅ 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
✅ we can fix it on our side We just need to add
✅ All those cycle imports are coming from our codebase. But probably fixing those will require major refactor how we import other libs.
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:
Next PR will fix
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 |
This is a great breakdown, thank you! |
Hi! Update on this: |
|
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:
|
Hi ! Update on this: |
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! |
@dangrous yes review is needed but from C+ reviewer firstly |
Hey, update on that 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. Web and Desktop
"shadow*" style props are deprecated. Use "boxShadow". Desktop❌ Errors
Native❌ Errors
A lot of cycle imports so I will not list them all No native ExponentConstants module found, are you sure the expo-constants's module is linked properly? |
Payment Summary
BugZero Checklist (@mallenexpensify)
|
@dangrous, @eVoloshchak, @mallenexpensify, @kubabutkiewicz Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Contributor+: @eVoloshchak due $250 via NewDot
@kubabutkiewicz, sounds good to me, do you want to keep working off this GH or start a new one? |
@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 |
@kubabutkiewicz new issue here. Please comment so I can assign to you. |
@dangrous, @eVoloshchak, @mallenexpensify, @kubabutkiewicz Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@dangrous, @eVoloshchak, @mallenexpensify, @kubabutkiewicz Still overdue 6 days?! Let's take care of this! |
@kubabutkiewicz, can we close this?!?! |
@mallenexpensify Yeah we can close it! |
Apologies for janky formatting below. The post in #expensify-open-source is easier to read
🧹 Some clean-up report on console errors / warnings as mentioned by @carlos here, quoting:
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)
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:86Animated: '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:28Android / 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:32Require 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
Issue Owner
Current Issue Owner: @kubabutkiewiczThe text was updated successfully, but these errors were encountered: