Upgrade to react-native-render-html@6.0.0-beta.8

[jsamr]

@marcaaron

Details

Upgrade react-native-render-html from 6.0.0-alpha.10 to version 6.0.0-beta.8 and adapt code to API changes. Great care was given on manually testing each platform. A visual comparision (before, after) for each platform is available in the Screenshots section.

Fixed Issues

#3951

Tests

1. Do npm ci to sync your node_modules
2. Start metro server and make sure to reset cache npm run start -- --resetCache
3. Run the native app
4. Open a discussion
5. Verify that all tags are displayed equally before and after patch (inline code, code fences, blockquote, italic, bold, anchors, edit tags and images)
6. Test manually if the image thumbnail button works as expected

QA Steps

Follow steps 4 to 6 from the previous section.

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

I could not run dev on dekstop, there seems to be a file missing in the dist folder (could reproduce prior and after patch):

[Renderer] [Error: ENOENT: no such file or directory, open '/Users/jsamr/Programmation/Expensify.cash/dist/version.json'] {
[Renderer]   errno: -2,
[Renderer]   code: 'ENOENT',
[Renderer]   syscall: 'open',
[Renderer]   path: '/Users/jsamr/Programmation/Expensify.cash/dist/version.json'
[Renderer] }
[Renderer] webpack-dev-server --config config/webpack/webpack.dev.js --port 8080 --platform desktop exited with code 1
--> Sending SIGTERM to other processes..

Screenshots

Web

Before patch	After patch

Mobile Web

Before patch	After patch

Desktop

N/A

iOS

Before patch	After patch

Android

Note that the inline code issue is being tracked here: #4005

Before patch	After patch

[marcaaron]

Sorry for the delay @jsamr. I'll give this a look tomorrow!

[jsamr]

@marcaaron No worries :-)

[marcaaron]

It looks like there is a very minor styling difference where hyperlinks now have a line underneath them. But I'm not sure if this is at all a blocker. Going to double check with @Expensify/design just in case there are strong opinions :)

Edit: Actually looks like this only happens on mobile web. I'm not sure if this is already like this but probably is and isn't a blocker.

Code looks good, but will do some testing locally before merging.

[megankelso]

I think the underlined hyperlink is super common and don't have any issues with it

[michelle-thompson]

Hmm, what if we didn't underline the hyperlink unless you're hovering over it? GH, for example, does this (I'm hovering over first link):

[marcaaron]

Not sure if I'm holding something wrong, but if I upload an attachment on iOS simulator I end up seeing this error message:

Once there I cannot make it go away and the attachment I uploaded will not display.

[marcaaron]

Issue also present on web

[jsamr]

@marcaaron did you run npm ci and cleared the cache? Those warnings look alpha-ish to me

[jsamr]

@marcaaron I'll take a look at why there is suddenly underline for web (but not web mobile, did I get the screenshot wrong?) tomorrow morning and post my findings here. If you'd like a hover rule as suggested by @michelle-thompson, I have to say IDK how to achieve that with react-native-web!

[marcaaron]

I deleted node modules and re-installed everything + cleared the metro cache

[jsamr]

@marcaaron That's really weird; cloning the branch in a tmp to start with a pure state, and I'll let you know if I can reproduce. In the meantime (cloning is slow!), if you npm why react-native-render-html, can you confirm it outputs the beta-8?

EDIT npm why is available in npm 7, might not be your case...

[marcaaron]

Ok, let me know how it goes. We are using node v14.16.0 & npm 6.14.11.

[jsamr]

@marcaaron
Unfortunately I could not reproduce from a fresh clone / install. Here is a screen recording for attaching an image

expensify-test.mp4

Most likely a cache issue... sorry those are so annoying! Could you try a clean state with

git clone git@github.com:jsamr/Expensify.cash.git --branch=rnrh-next

EDIT my recording tool (Peek) added frames from the end in the beginning... weird bug

[marcaaron]

So, running in the cloned repo seems to have worked and is testing well. But breaks stuff when pulling this branch from my main project folder. Maybe it's a gremlin - but I just want to verify this isn't going to cause issues for everyone who merges the main branch in after we merge this. Will do a bit more testing.

[marcaaron]

Ok all resolved on my end ::oh-nothing:: !
LGTM just need to fix the conflicts 👍

[marcaaron]

I think npm ci maybe did something. but react native community cli is suddenly not working 😭

➜  Expensify.cash git:(f8f663aec) ✗ react-native run-ios --verbose
error Option "--scheme" is missing
Error: Option "--scheme" is missing
    at /Users/marcaaron/Expensidev/Expensify.cash/node_modules/@react-native-community/cli/build/tools/assertRequiredOptions.js:51:13
    at Array.forEach (<anonymous>)
    at assertRequiredOptions (/Users/marcaaron/Expensidev/Expensify.cash/node_modules/@react-native-community/cli/build/tools/assertRequiredOptions.js:40:11)
    at Command.handleAction (/Users/marcaaron/Expensidev/Expensify.cash/node_modules/@react-native-community/cli/build/index.js:181:42)
    at Command.listener (/Users/marcaaron/Expensidev/Expensify.cash/node_modules/commander/index.js:315:8)
    at Command.emit (events.js:315:20)
    at Command.parseArgs (/Users/marcaaron/Expensidev/Expensify.cash/node_modules/commander/index.js:651:12)
    at Command.parse (/Users/marcaaron/Expensidev/Expensify.cash/node_modules/commander/index.js:474:21)
    at setupAndRun (/Users/marcaaron/Expensidev/Expensify.cash/node_modules/@react-native-community/cli/build/index.js:267:24)
    at run (/Users/marcaaron/Expensidev/Expensify.cash/node_modules/@react-native-community/cli/build/index.js:206:11)

building from x code worked fine

[jsamr]

Oh jeez; npm nutery! ; AFAIK ci stricly matches npm lockfile while install allows more recent dependencies versions... perhaps try upgrade cli... sorry for this nightmare, I can take a look tomorrow morning and test on my Mac...

[jsamr]

@marcaaron So I tried on main branch on my mac, commit 329dc18

First, with npm install followed by cd ios; pod install:

Error: Option "--scheme" is missing

Then with npm ci followed by cd ios; pod install:

Error: Option "--scheme" is missing

So I believe the issue is not related to my commit. I guess it didn't occur to me when I tested because I built and run from XCode directly.

[parasharrajat]

Look Good. One Question why aren't we upgrading to 6.0.2? Asking just for curiosity?

Also, one question about dangerouslyDisableWhitespaceCollapsing={false}.
If we set it to true, What would be the major change in the output apart from the spaces that will not collapse. Will it break the layout?

Thanks for your time.

[jsamr]

Look Good. One Question why aren't we upgrading to 6.0.2? Asking just for curiosity?

6.0.2 was not released at the time of writing this PR. But there are no functional differences between 6.0.0-beta.8 and 6.0.2.

If we set it to true, What would be the major change in the output apart from the spaces that will not collapse. Will it break the layout?

It shouldn't break the layout. Basically, would be equivalent to setting whiteSpace: "pre" in baseStyle

[marcaaron]

So I believe the issue is not related to my commit. I guess it didn't occur to me when I tested because I built and run from XCode directly.

Got it, I think perhaps related to including expo unimodules but unsure. App builds fine in Xcode and Android Studio.

We still need to fix the conflicts here then I can merge :)

[jsamr]

@marcaaron Sorry I was unsure if you needed me to do the resolution; but I just realized that you were probably waiting for me to do it. So I just did it!

[OSBotify]

🚀 Deployed to staging in version: 1.0.79-6🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production in version: 1.0.80-2🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅