Skip to content
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

Use remarkable for Markdown formatting to support React Native #283

Merged
merged 2 commits into from
May 1, 2024

Conversation

opiation
Copy link
Member

Contributes to #139

This PR uses the remarkable library to transform Markdown into HTML instead of marked. While marked is a competent library, it uses regular expressions to parse Markdown which include a syntax currently unsupported by the Hermes JS engine. In contrast, remarkable does use any regular expressions to parse so it seems to work just fine in React Native.

After switching to remarkable, using <FhirValue> in an Expo application to display Markdown content in HTML resulted in a new error. This turned to be a straightforward issue that the dompurify library cannot sanitize an HTML string with access to the DOM. Thus, this PR narrows HTML sanitizing to only be done when dompurify can do so.

How can I test this change?

Create a new Expo app using the bonfhir template for it. Add a <FhirValue> with some example Markdown content styled as HTML and validate that the HTML shows up literally on the Android screen.

const exampleMarkdown = "# Footer\n\nThis is a footer";

function ExampleScreen() {
  return <FhirValue
    options={{ style: "html" }}
    type="markdown"
    value={exampleMarkdown}
  />
}

The above should show something like:

<h1>Footer</h1>
<p>This is a footer</p>

Caveat

This helps make @bonfhir/core and @bonfhir/react more compatible with React Native configured with Hermes by addressing specific issues. Full Hermes compatibility is not confirmed yet however as testing the complete surface of Bonfhir libraries and APIs is out of scope of this fix. We welcome the community finding more compatibility issues however that we can prioritize and address as needed!

…S engine

While using `remarkable` for Markdown formatting is supported in the
Hermes JS engine, it requires instantiation of the `Remarkable` class
before using it. To avoid any surprises from possibly instantiating the
class throwing an exception, this change lazily instantiates the class
once on first use.

This dependency also increases the overall bundle size unfortunately:

Bundle size changes (Gzipped)
- @bonfhir/core/r4b CommonJS: 70.53 KB -> 85.99 KB
- @bonfhir/core/r4b ES module: 69.68 KB -> 85.14 KB
- @bonfhir/core/r4b Global: 70.16 KB -> 85.64 KB
- @bonfhir/core/r5 CommonJS: 73.76 KB -> 89.25 KB
- @bonfhir/core/r5 ES module: 72.88 KB -> 88.32 KB
- @bonfhir/core/r5 Global: 73.43 KB -> 88.91 KB
DOMPurify returns early in its core business logic instantiation so when
a DOM is not available ostensibly because it relies on the DOM to parse
the given HTML string. To support React Native, we need to only sanitize
the HTML string when `DOMPurify` supports it.

This allows `@bonfhir/react`'s `FhirValue` component to be used in React
Native applications though by making the application developer
responsible for making sure the rendered content is safe in such a case.
Rendering Markdown content as HTML didn't work at all before in React
Native so this is a positive change though I haven't fully explored the
real vulnerability of rendering Markdown or HTML into a React Native
app.
@opiation opiation added bug Something isn't working react-native labels Apr 29, 2024
@opiation opiation marked this pull request as ready for review April 29, 2024 21:11
@opiation
Copy link
Member Author

opiation commented May 1, 2024

@julienblin, any objections to merging this? Is there any additional test you'd like me to run before merging this?

Copy link
Contributor

@lp lp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

@opiation
Copy link
Member Author

opiation commented May 1, 2024

@julienblin, I'm merging this now. This change should be trivial to revert if it's revealed to be a problem.

@opiation opiation merged commit bf85c5d into main May 1, 2024
1 check passed
@opiation opiation deleted the refactor/support-custom-markdown-formatter branch May 1, 2024 20:36
@github-actions github-actions bot mentioned this pull request May 1, 2024
opiation added a commit that referenced this pull request May 2, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @bonfhir/core@2.19.4

### Patch Changes

- [#283](#283)
[`6948f7a`](6948f7a)
Thanks [@opiation](https://github.com/opiation)! - Use `remarkable`
instead of `marked` to format Markdown content. This avoids using
regular expression syntax that isn't yet supported by the Hermes JS
engine, allowing `@bonfhir/core` to load without error in Expo / React
Native applications.

## @bonfhir/react@3.2.2

### Patch Changes

- [#283](#283)
[`3547a0f`](3547a0f)
Thanks [@opiation](https://github.com/opiation)! - Only sanitize HTML
output from Markdown formatting when formatted where a DOM is available
(browser, Deno, etc.). Platforms without a DOM (React Native) have other
means of rendering and sanitizing a given Markdown or HTML string into a
relatively safe and usable UI.

- Updated dependencies
\[[`6948f7a`](6948f7a)]:
    -   @bonfhir/core@2.19.4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working react-native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants