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

fix(app): replace remark with react-markdown #14942

Merged
merged 2 commits into from
Apr 17, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Apr 17, 2024

Closes RQA-2587 and RQA-2566

Overview

After the vite migration, the app would crash any time the release notes markdown file needed parsing, because remark decides to invoke process.cwd() for some reason. Webpack can mock "process" out, but there's no 1:1 analogous solution with Vite.

Although there's probably some hacky way do accomplish the same thing, remark-react has been deprecated for over three years, so that's more incentive to migrate.

After testing a few options, react-markdown is relatively lightweight and properly overrides the markdown as desired (ie, no additional changes need to be made to the markdown for the refactor). It also doesn't have the same Vfile lib dependency that is the source of the "process" issue, so no more crashing when parsing release notes. It's also well-maintained, etc.

Screenshot 2024-04-17 at 5 23 41 PM

Test Plan

  • Pull down the branch and build the app locally. You may fail key signing, and that's ok. It will still work.
  • You should see robot update banners. Click on update and verify that the release notes appear instead of the app white screening.

Changelog

  • Updating the app or a robot no longer causes a whitescreen when the release sh

Review requests

  • Did I add this package correctly? Are there any other places I'm missing?

Risk assessment

low

mjhuff added 2 commits April 17, 2024 17:10
After the vite migration, the app would crash any time the release notes markdown file needed to
parsed, because remark decides to invoke process.cwd() for some reason. Webpack mocks process out,
but vite doesn't. Although there's probably some hacky way do accomplish the same thing in vite,
remark-react has been depricated for over two years, so that's more incentive to migrate. After
testing a few options, react-markdown is relatively lightweight and properly overrides the markdown
as desired (ie, no additional changes need to be made to the markdown for the same end result). It
also doesn't have the same Vfile lib dependency that is the source of the "process" issue, so no
more crashing when parsing release notes.
@mjhuff mjhuff requested review from sfoster1 and shlokamin April 17, 2024 21:20
@mjhuff mjhuff requested review from a team as code owners April 17, 2024 21:20
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks good and correct and not missing anything!

@mjhuff mjhuff merged commit c096931 into edge Apr 17, 2024
21 checks passed
@mjhuff mjhuff deleted the app_fix-process-reference-error branch April 17, 2024 22:23
@mjhuff mjhuff restored the app_fix-process-reference-error branch April 23, 2024 22:07
@mjhuff mjhuff deleted the app_fix-process-reference-error branch May 3, 2024 00:40
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
Closes RQA-2587 and RQA-2566

After the vite migration, the app would crash any time the release notes markdown file needed parsing, because remark decides to invoke process.cwd() for some reason. Webpack can mock "process" out, but there's no 1:1 analogous solution with Vite.

Although there's probably some hacky way do accomplish the same thing, remark-react has been deprecated for over three years, so that's more incentive to migrate.

After testing a few options, react-markdown is relatively lightweight and properly overrides the markdown as desired (ie, no additional changes need to be made to the markdown for the refactor). It also doesn't have the same Vfile lib dependency that is the source of the "process" issue, so no more crashing when parsing release notes. It's also well-maintained, etc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants