-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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.
@julienblin, any objections to merging this? Is there any additional test you'd like me to run before merging this? |
opiation
commented
May 1, 2024
lp
approved these changes
May 1, 2024
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.
💪
@julienblin, I'm merging this now. This change should be trivial to revert if it's revealed to be a problem. |
Merged
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributes to #139
This PR uses the
remarkable
library to transform Markdown into HTML instead ofmarked
. Whilemarked
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 thedompurify
library cannot sanitize an HTML string with access to the DOM. Thus, this PR narrows HTML sanitizing to only be done whendompurify
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.The above should show something like:
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!