Repository navigation
[Hold for Payment 30th July] Upgrade react-native-render-html to latest (6.0.0-beta.8) #3951
Description
This is a follow-up on a conversation I had with @puneetlath. The Foundry release has entered beta stage recently, and thus has stabilized a lot. I am confident that the official 6.0.0 release should be ready at some point this summer. Relevant resources to read: Announcing the Beta Foundry Release, Changelog
Benefits
Currently, Expensify.cash is using the 6.0.0-alpha.10 version, which lags behind a few important breaking changes and enhancements. The main advantages of integrating this new release:
- Test coverage has been risen significantly in the main repository (jest reports > 98%)
- API has stabilized a lot and it not expected to change before final release.
- Many little performance improvements (check the changelog referred above).
- The API is documented in depth which is great for contributors, check out documentation website.
Planning
- Upgrade version to latest beta and adapt code where required to keep the same level of functionality;
- Test all tags to make sure nothing breaks;
- Review best practices regarding react-native-render-html usage, and include refactorings to enforce those best practices.
Upwork job post: https://www.upwork.com/jobs/~01214906855dec6aa3
Reacted by Puneet Lath
Activity
MelvinBot commentedon Jul 9, 2021
Triggered auto assignment to @johncschuster (
AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.parasharrajat commentedon Jul 9, 2021
Thanks, @jsamr for such a great lib. I am moving with a solution on PR #3869.
But I am not covering
Review best practices regarding react-native-render-html usage, and include refactorings to enforce those best practices..MelvinBot commentedon Jul 9, 2021
Triggered auto assignment to @marcaaron (
Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.jsamr commentedon Jul 9, 2021
@parasharrajat Thank you for this update! As I've just stated in your PR, I don't think disabling whitespace collapsing is a good idea since originally I was hired to implement whitespace collasing in the engine! Moreover, the amount of testing should be significant since we must make sure all tags are rendered as expected.
parasharrajat commentedon Jul 9, 2021
Ok. I see. But we want to fix the spaces issue. Any solution for that issue. Holding that PR for further investigation.
jsamr commentedon Jul 9, 2021
@parasharrajat Preserving spaces for the specific use case laid out it #3859 is really a matter of design choice, so it should be discussed with a product owner. If you type 3 spaces in this very Github markdown editor, spaces will be collapsed. To preserve spaces, one should explicitly use a preserving space block such as
<pre>(or create a custom tag which has awhitespace: 'pre'style applied to it). I don't know of any chat app that behaves like this, but as I said, it's just a matter of opinion. In any case, that should be handled at the Markdown → HTML conversion level!parasharrajat commentedon Jul 9, 2021
Yeah. Clearly. But there was no straight way to do it. We may need to add
<p>Tags on our parsing and then break each line of text to<p>. Not sure if this works.22 remaining items
puneetlath commentedon Jul 26, 2021
Reopening to ensure this gets paid out if no regressions.
parasharrajat commentedon Jul 26, 2021
There is one regression on this PR. If we resize the browser app crashes. Somehow HTML source is being set to
undefinedand shallow matching of the prop is failing where it's trying to covert the undefined to object.jsamr commentedon Jul 26, 2021
@parasharrajat Yeah I think the current version is stricter and doesn't allow
undefined. But since thehtmlprop is required in the surrounding componentBaseRenderHTML, I would assume that is the contract for this component.App/src/components/RenderHTML/renderHTMLPropTypes.js
Line 5 in 3d35169
As I have not touched other parts aside from
src/components/RenderHTML, I would guess the bug comes from another component consuming this one.[-]Upgrade react-native-render-html to latest (6.0.0-beta.8)[/-][+][Hold for Payment 30th July] Upgrade react-native-render-html to latest (6.0.0-beta.8)[/+]parasharrajat commentedon Jul 27, 2021
@jsamr Thanks for the context. I will look into it.
parasharrajat commentedon Jul 27, 2021
Sorry for the confusion but it seems that was no issue.
npm installfixed it.marcaaron commentedon Jul 29, 2021
Melvin shoo!
trjExpensify commentedon Jul 31, 2021
Paid.