-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Performance: use React Native Render HTML explicit composite architecture #4502
Conversation
@marcaaron I am planning to do two benchmarks as we discussed in #4123; the only thing I am unsure is this:
Can you provide me guidance on how to "log to your log server", and how to check those logs? |
Ah to clarify, only Expensify staff can check these logs. But it's something I can help you with no problem. |
Ok! Did some pretty basic benchmarking here on an Android release build. Unfortunately, I didn't see a huge difference in how long it takes to lists of chat messages. This PR is maybe 10 ms faster than the main branch - which seems inconclusive. Not sure whether you have other ideas about what to test? On the positive side, things didn't get slower and everything appears to work fine so I can't think of a reason not to recommend these changes. |
@marcaaron The thing is that |
Got it. That makes sense. I'm not sure there is any requirement that we load 100 or more items at once (at least not yet), but it's good to know it will be faster if we did. |
This allows to avoid the cost of instantiating a new engine for each HTML snippet, see https://git.io/JRcZb fix Expensify#4123
Self-closing tags are only allowed for void elements in HTML5. Otherwise, the required behavior for parsers is to ignore the slash and consider the expression as a tag opening. This will lead to bugs with htmlparser2 if `<edited />` was followed by other tags, where those tags would be parsed as children of `<edited>` instead of siblings.
@marcaaron Great! I fixed some conflicts with main and marked this PR as ready for review. Tell me if there is anything else I should do! |
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.
No changes to request.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @marcaaron in version: 1.0.85-10 🚀
|
This has been deployed to production and is now subject to a 7-day regression period. |
🚀 Deployed to production by @roryabraham in version: 1.0.86-11 🚀
|
This has been deployed to production and is now subject to a 7-day regression period. |
@marcaaron
Details
Summary of changes:
HTMLEngineProvider
, has been created. This component rendersTRenderEngineProvider
andRenderHTMLConfigProvider
. All configuration props, including custom renderers, are now handled by this component, which is rendered at the root of the application (App.js).RenderHTML
component has been greatly simplified to a one-file component renderingRenderHTMLSource
component.<edited>
element (see commit).This refactoring follows closely guidance provided in the docs: https://git.io/JRcZb
Fixed Issues
#4123
Tests
There are no behavioral changes. If something should not work, it would break immediately.
QA Steps
N/A
Tested On
Screenshots
N/A