-
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
Console error cleanup: patch react-native-render-html #52917
Conversation
@paultsimura Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
In #49035, we've decided we'll hold for the upstream fix, which is being worked on (but super slow, ngl): meliorence/react-native-render-html#674 @lakchote did we change the decision? Should we apply the patch? |
We didn't change decision, as far as I know. I'm not fond of doing patches, except if it's for a critical issue/use case. It was decided here to wait for the upstream fix. cc @blazejkustra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment, besides LGTM.
@@ -22,4 +22,4 @@ index 6ac80d8..0000000 | |||
- s.source_files = "ios/**/*.{h,m}" | |||
- | |||
- s.dependency 'React-Core' | |||
-end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a last empty line.
@JKobrynski @rezkiy37 please see #52917 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JKobrynski I wonder if we need to change the .js.map
files for this patch 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we revert any changes here @JKobrynski ?
|
Closing the PR as it was decided to wait for the upstream to be fixed. |
Explanation of Change
This PR introduces a patch to the
react-native-render-html
library that gets rid of the following console warning we've been seeing on the web/desktop:It basically removes all occurrences of
.defaultProps
and related code from the TS, JS and JS map files in the library.Here is a related issue in the package's repository. Based on the discussion it seems like the library won't be updated anytime soon (the last release was ~2 years ago) and the only solution the community has at the moment is applying a patch.
Fixed Issues
$ #49035 #51099
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
Same as Tests section above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
MacOS: Chrome / Safari
MacOS: Desktop