React Native Render HTML: whitespace collapsing

[jsamr]

Summary

• Upgrade react-native-render-html to version 6.0.0-alpha.7 6.0.0-alpha.8
• Refactored src/styles/StyleSheet (styles are passed to custom renderers, thus there is no need to define distinct styles for code and pre tags)
• Moved HTML rendering logic from src/pages/home/report/ReportActionItemFragment.js to the RenderHTML component, adapted to RNRH v6
• Refactored InlineCodeBlock and adapted its logic to the new API
• Refactored propTypes for AnchorForCommentsOnly
• Remove react-native-render-html from files transpiled by babel-loader (not required anymore, the library already transpiles JSX)
• Added __DEV__ global variable in webpack environment, required by react-native-render-html, see https://reactnative.dev/docs/javascript-environment

Notes

• I disabled inline CSS parsing with the new enableCSSInlineProcessing prop. If you think you might need this feature, I can enable it.
• The new release requires consumers to register available fonts in the app. Use systemFonts for that purpose (I've already registered all the fonts declared in src/styles/fontFamily).
• There is no official documentation for the new release yet. However, being written in Typescript, you can benefit from intellisense and inspect declaration files easily. Also, you can take a look at snippets from the "Foundry Playground" demo, especially on how custom renderers work. All the steps are detailed here: [6.x] 🚀 Welcoming the Foundry release! meliorence/react-native-render-html#430

Suggestions

• I noticed you use a lot the Dimension API. It is known for being unreliable, (see Make a decision about the Dimensions module react-native-community/discussions-and-proposals#291). I recommend useWindowDimensions hook instead!
• .eslintrc.js is in config folder, which is non standard. Would be best if the linter config could be picked automatically by VSCode, very handy for format action!
• There is a plugin for estlint + prettier. It is efficient to avoid conflicting requirements.

Future work

• I didn't refactor the image component since it works as it is. However, I recommend to reuse the internal renderer for images, which handles scaling very well. For that purpose, I initially thought you could use DefaultTagRenderer prop in the renderer component (he would require that you pass down all the props). But I realize that you tamper with the URL for credentials. Instead, you would need to reuse the internal HTMLImageElement component. I haven't exported that yet in the new API, but I will do so soon (see Use internal default renderers logic in custom renderers meliorence/react-native-render-html#424). I could work on a new PR as part of a distinct service if you are interested.
• I will release an important optimization from which the chat app could really benefit. That is currently, for every RenderHTML instance, a new instance of TRenderEngine is created, with a big tree of objects representing the underlying models for HTML tags and CSS properties, which in turns depends on configuration (custom renderers, styles ... Etc). I am planning to provide two additional React components:
  • TRenderEngineProvider, which will take config props and create a React context providing one engine instance;
  • RenderHTMLFragment, which will consume the engine and build the react tree from html / uri.

Considering the linear cost of re-instantiating an engine for each message in the thread, I truly believe it could make a very sensible addition.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/143758

Tests

Open a chat thread and make sure:

• Pre blocks are correctly rendered
• Inline code tags are correctly rendered
• Images thumbnail and modal work
• Anchors are correctly rendered

Screenshots

Android

Android (before)	Android (after)

iOS

iOS (before)	iOS (after)

Desktop

Desktop (before)	Desktop (after)

Web

I managed to run the dev server, but I couldn't get pass the login screen. The app complains it has no access to the API. I followed all steps detailed in this document, but looks like it wasn't enough.

[marcaaron]

Looks great overall and thanks for these changes! Left a couple of quick comments about style. I'll leave testing to @AndrewGable.

[jsamr]

@marcaaron Thanks for the feedback!! I'll wait for Andrew's input.

[AndrewGable]

Looking great! Two things:

1. Looks like there are some conflicts, could you fix those?
2. Can you run npm run lint, it looks like there are two super small lint issues. We usually would run these via GitHub Actions, but haven't set them up for forks quite yet.

[jsamr]

@AndrewGable I made the requested changes:

• renamed propTypes to anchorForCommentsOnlyPropTypes and similarly inlineCodeBlockPropTypes ;
• fixed lint errors;
• split InlinCodeBlock to platform-specific sources (.android, .ios);
• rebased on master and resolved conflicts (I added a comment in webViewStyles to encourage not to set distinct styles for custom renderers as tagsStyles are passed through the style prop).

Hope that's fine! I made a quick screenshot to make sure the result was identical (I wanted to test blockquote because a new custom renderer had been registered in master, but apprently the markdown parsing is not ready yet):

[AndrewGable]

Code is looking great, I am just working on some testing then I will !

[AndrewGable]

Tests great. Thanks for the awesome PR! 👏

It was a pleasure working with you on this issue, I'd love for you to implement your suggestions and I will chat with @puneetlath about next steps to write up another agreement so we can get those implemented if you are up for it.

[jsamr]

@AndrewGable Hey Andrew, the pleasure is shared! I would love to participate again. I'm currently working on the intermediary v5 release after which I'll have to port all new features (and deprecations) to v6. A new service could also be the opportunity for me to adapt this PR code to the yet evolving shape of the V6 API. To reassure you, I really think it will have greatly stabilized by the time of my next intervention.

[AndrewGable]

@jsamr - It looks like we are getting an error when we deployed this to production:

text-transforms.js:19 Uncaught SyntaxError: Invalid regular expression: /([⺀-⻿⼀-⿟⿰-⿿　-〿�-ゟ゠-ヿ㄰-��-㆟㇀-㇯ㇰ-ㇿ㌀-��-䶿一-鿿ꀀ-��-�豈-﫿�-︟︰-��-﹯＀-￯])
([⺀-⻿⼀-⿟⿰-⿿　-〿�-ゟ゠-ヿ㄰-��-㆟㇀-㇯ㇰ-ㇿ㌀-��-䶿一-鿿ꀀ-��-�豈-﫿�-︟︰-��-﹯＀-￯])/: Range out of order in character class
    at new RegExp (<anonymous>)
    at Object.<anonymous> (text-transforms.js:19)
    at n (bootstrap:19)
    at Object.<anonymous> (collapse.js:14)
    at n (bootstrap:19)
    at Object.<anonymous> (TRenderEngine.js:8)
    at n (bootstrap:19)
    at Object.<anonymous> (index.js:170)
    at n (bootstrap:19)
    at Object.<anonymous> (TNodeRenderer.js:10)

Do you have any ideas on this one?

[AndrewGable]

Looks like I should not have marked this comment as "off topic": #851 (comment)

I can reproduce this by:

1. npm run build
2. Opening ReactNativeChat/dist/index.html

Looking into what's different between dev and prod webpack configs that could cause this.

[jsamr]

@jsamr - It looks like we are getting an error when we deployed this to production:

text-transforms.js:19 Uncaught SyntaxError: Invalid regular expression: /([⺀-⻿⼀-⿟⿰-⿿　-〿�-ゟ゠-ヿ㄰-��-㆟㇀-㇯ㇰ-ㇿ㌀-��-䶿一-鿿ꀀ-��-�豈-﫿�-︟︰-��-﹯＀-￯])
([⺀-⻿⼀-⿟⿰-⿿　-〿�-ゟ゠-ヿ㄰-��-㆟㇀-㇯ㇰ-ㇿ㌀-��-䶿一-鿿ꀀ-��-�豈-﫿�-︟︰-��-﹯＀-￯])/: Range out of order in character class
    at new RegExp (<anonymous>)
    at Object.<anonymous> (text-transforms.js:19)
    at n (bootstrap:19)
    at Object.<anonymous> (collapse.js:14)
    at n (bootstrap:19)
    at Object.<anonymous> (TRenderEngine.js:8)
    at n (bootstrap:19)
    at Object.<anonymous> (index.js:170)
    at n (bootstrap:19)
    at Object.<anonymous> (TNodeRenderer.js:10)

Do you have any ideas on this one?

Yes, absolutely https://github.com/native-html/core/blob/bee4e1124449c993c0995b87a0c79316e20a6274/packages/transient-render-engine/src/flow/text-transforms.ts#L10
These character-sets are east-asian unicodes which must be handled in a specific way according to CSS standard. But React Native has proven to handle poorly some locale-specific issues with strings (they don't support unicode classes in regexes for example). This seems to be another instance ... On which platform is the issue happening? Are you using hermes? OK sorry, I just realize the issue happens with react-native-web. I guess it could be an issue with webpack ...

In the meantime, I'll publish a hotfix tomorrow in @native-html/transient-render-engine, disabling this behavior for east-asian characters, and bring it up here. Shall I open a distinct new PR?

[jsamr]

@AndrewGable I don't know whitch minifier you use with webpack, but I've seen people complaining about a similar issue with terser, see vuejs/vue#9456 (comment), or babel, see babel/babel#5564 (comment)

My assumption would be that the minifier replaces '\uxxxx' sequences in strings with their unicode counterpart to save space, but that breaks regex instantiation. I think I'll just put the unicode sequences directly in the regex to prevent such minifier mangling, because if that's the issue, you won't be the last to face it! RIP DRY

[AndrewGable]

Yes we ran into this on web, using react-native-web. You can open up a new PR with this and the latest fixes as we reverted this PR specifically. That's an annoying bug!

p.s. No rush on this! My fault for not testing a production build before deploying it 😄