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

Support custom JS parsers without having separate bundle #317

Closed
tomekzaw opened this issue Apr 16, 2024 · 17 comments
Closed

Support custom JS parsers without having separate bundle #317

tomekzaw opened this issue Apr 16, 2024 · 17 comments
Assignees
Labels
enhancement New feature or request

Comments

@tomekzaw
Copy link
Collaborator

tomekzaw commented Apr 16, 2024

Currently, Live Markdown comes with a JS parser/formatter that wraps ExpensiMark and converts Markdown message into a list of ranges (style, location, length). This code needs to be run on the UI runtime while typing so it is executed on a separate JS runtime. Currently, this code is distributed as a separate bundle which is loaded from this file distributed along with the Live Markdown library: https://github.com/Expensify/react-native-live-markdown/blob/main/parser/react-native-live-markdown-parser.js

However, this makes things more difficult to maintain since after each change in expensify-common we need to bump it in @expensify/react-native-live-markdown and then bump both these libraries in Expensify/App repo. This also leads to inconsistencies when E/App and Live Markdown use different versions of expensify-common.

Finally, since the JS parser/formatter is bundled, there's no way to parametrize its behavior based on app state, however in some cases this will be necessary, e.g. to implement:

  • short mentions (we need to pass a list of previously mentioned users)
  • correct background color for user mentions when the current user is mentioned (should be green instead of blue)

The objective of this task is to allow passing JS parser/formatter as a JS function directly to MarkdownTextInput component. Here's the current idea for the API:

function App() {
  const parser = useMarkdownParser((markdown: string) => {
    'worklet';
    // some code here
    return [
      {type: 'syntax', start: 2, length: 1},
      {type: 'bold', start: 3, length: 5},
      {type: 'syntax', start: 8, length: 1},
    ];
  }, []);

  return <MarkdownTextInput
        multiline
        autoCapitalize="none"
        value={value}
        onChangeText={setValue}
        style={styles.input}
        ref={ref}
        markdownStyle={markdownStyle}
        parser={parser}
        placeholder="Type here..."
        onSelectionChange={(e) => setSelection(e.nativeEvent.selection)}
        selection={selection}
      />;
}

The current implementation of JS parser/formatter will be exported as the following hook:

const parser = useExpensiMarkParser();
@tomekzaw tomekzaw added the enhancement New feature or request label Apr 16, 2024
@tomekzaw tomekzaw self-assigned this Apr 16, 2024
@tomekzaw tomekzaw changed the title Support custom JS parsers without separate bundle Support custom JS parsers without having separate bundle Apr 16, 2024
@tomekzaw
Copy link
Collaborator Author

tomekzaw commented May 20, 2024

@mallenexpensify
Copy link

@tomekzaw do you have the bandwidth to prioritize this? If not, wanna see about tag-teaming it with another engineer or finding a volunteer to take over? I know you posted the P/S just yesterday, there was a month+ before that without an update, which is why I'm asking.

@tomekzaw
Copy link
Collaborator Author

@mallenexpensify Actually, I have a working proof of concept that uses Reanimated worklets (Reanimated is already a dependency so that's cool) so we might be able to use that quite soon. The only problem for the time being is that ExpensiMark still uses jQuery in some places, but this is something that can be parallelized or even made external, what do you think?

@mallenexpensify
Copy link

@tomekzaw , honestly, I have no clue. I'm not familiar with this. Looks like a plan was hatched in this thread and that Expensify/App#42494, linked above, is the next step. thx.

@tomekzaw
Copy link
Collaborator Author

Working on it, looks promising, will share a video recording with a demo early next week.

@shubham1206agra
Copy link

@tomekzaw Is there any update here?

@tomekzaw
Copy link
Collaborator Author

Latest update here

@szado
Copy link

szado commented Jul 18, 2024

@tomekzaw can you write a short update here for open source users without access to SM Slack?

@tomekzaw
Copy link
Collaborator Author

Sure, will do.

TL;DR in an upcoming version of react-native-live-markdown, MarkdownTextInput component will expose a parser prop that accepts a JS function (must be a worklet, since it's run on the UI thread as you type) and returns an array of ranges (like {start: 2, length: 4, type: 'link'}). This way it will be possible to implement custom formatting logic full in JS with fast-refresh support.

Then, in a following step, instead of having a predefined list of syntax elements (like bold, link or mention-here) customizable only via markdownStyle, it will be possible to use CSS styles (like color, backgroundColor, fontSize, fontWeight, fontStyle etc.), but this is a separate feature.

@szt217
Copy link

szt217 commented Sep 13, 2024

Hello @tomekzaw,

I've been testing out the branch for PR #439 on iOS and the custom parsing is working great. Having an of issue with the Android dev build and I see you were working on it today. Do you have an idea on the timeline for when this PR might be merged?

Appreciate all that you do here.

@tomekzaw
Copy link
Collaborator Author

Hi @szt217, thanks for testing out the PR! Happy to hear that custom parsing works well for you.

I've faced some issues with incorrect MarkdownTextInput height on RN 0.75 on Android with new arch enabled.

Realistically, I'd expect this PR to be merged next month. There are still some bugs to fix and it needs to be properly reviewed since there's a lot of files changed.

@puneetlath
Copy link

Hello! Just curious how this is going?

@tomekzaw
Copy link
Collaborator Author

tomekzaw commented Oct 31, 2024

@puneetlath We're almost there. The feature is already implemented, the PR has been reviewed by @j-piasecki and we're waiting for a good time to merge it (we were still fixing some minor bugs in web implementation but now it's done).

There's a plan to use live-markdown to highlight search query so we'll be aiming to land this feature quite soon. Currently, @Kicu is reviewing the PR. Will keep you posted!

@puneetlath
Copy link

Great!

@tomekzaw
Copy link
Collaborator Author

tomekzaw commented Dec 4, 2024

@puneetlath I'm glad to announce that today we finally merged #439. This is a milestone in the development of the library.

What does it mean for us now?

  • We no longer need to bump react-native-live-markdown library each time ExpensiMark from expensify-common changes. From now on, MarkdownTextInput will use expensify-common from E/App. This means we can bump these two libraries in E/App more independently. Also, we will avoid parsing inconsistencies caused by using two different versions of expensify-common.
  • We can now parametrize parsing process with external data like app state. For instance, it will be possible to correctly implement short mentions. We can unblock [HOLD #53627] Render shortened mentions with blue/green outline in live markdown preview App#38025.
  • We can re-use MarkdownTextInput component for other use-cases like highlighting some keywords in search query. We can unblock [Search v2.5] [Autocomplete] Highlight autocomplete key and selected value App#50949.
  • We will be able to debug and profile the parsing algorithm using the same tools as for debugging and profiling the main JS bundle.
  • We will be able to move the parsing algorithm from react-native-live-markdown to Expensify/App so its code is tracked the same way as the app code.

What do we need to do now?

  • Obviously, we need to bump react-native-live-markdown in Expensify/App to 0.1.188 (or later). @289Adam289 from Software Mansion will submit a PR with the bump.
  • There are some parts of the code I'd like to refactor but I wanted to keep them separate from this PR – I will submit them as follow-ups.

Thanks!

@tomekzaw
Copy link
Collaborator Author

tomekzaw commented Dec 4, 2024

Done in #439.

@tomekzaw tomekzaw closed this as completed Dec 4, 2024
@puneetlath
Copy link

That's awesome!! Thanks for the update 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: CRITICAL
Development

No branches or pull requests

6 participants