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

Option to use parsed Markdown #123

Closed
wants to merge 1 commit into from

Conversation

tananaev
Copy link

@tananaev tananaev commented Sep 7, 2023

Currently the library parses markdown internally, but there are several issues with it:

  • The parsing is asynchronous without any callbacks, which causes many problems. For example, it's not possible to scroll the list with markdown items to the right position because in the initial state all items are zero height
  • It's not possible to do parsing and related logic in the view model or data layer
  • It's not possible to support custom nodes (this is something we want to contribute later as well)

This PR adds an overload for Markdown composable that takes Commonmark Node object.

One drawback is that we need to expose Commonmark as an API dependency because public API now uses it, but feel like it should be ok considering that the module is called richtext-commonmark.

@halilozercan
Copy link
Owner

During the initial implementation of Markdown composable, we've had a similar discussion about exposing an alternative API where commonmark would be just another pluggable parser to a common Markdown Tree that we can define for ourselves. This way anyone could use their own parser or parser+translation combo to create their Markdown composable. Unfortunately, we never got the time to try this and the development stagnated heavily.

Would you be willing to introduce something like that? I know it's a way larger project in scope compared to this solution. Although the module itself is named richtext-commonmark, I still wouldn't feel easy about exposing that relation on the API surface.

@tananaev
Copy link
Author

tananaev commented Sep 8, 2023

Can you please clarify what the API would look like. Do you mean passing a parser function directly into composable? Wouldn't that require commonmark dependency as well? Also passing a parser wouldn't solve some of the problems related to asynchronous calls in the UI code.

One option I can think of is to make own data models that map to commonmark internally. That way we don't need to expose commonmark externally. Basically exposing AstNode and other classes.

@tananaev
Copy link
Author

tananaev commented Sep 9, 2023

@halilozercan any thoughts? I'm happy to modify the code, but need some clarification.

@tananaev
Copy link
Author

@halilozercan are we able to move forward with something? Any guidance on what you want to see as the API?

@dimitriz09
Copy link

Very interesting point of view. I personally think having a custom parser is not that important, but it's because the solution as it is now fits my needs.
The asynchronous loading is really a pain in lists though.

@dimitriz09
Copy link

@halilozercan Could you consider synchronized composition so we can use markdown in lazylists more efficiently ?
Regards,
Dimitri

@halilozercan
Copy link
Owner

I'm sorry for abandoning this issue and the discussion.

Here is what I had in mind;

  • richtext-markdown module would continue to expose a @Composable Markdown() with the addition of an AST model that's specific to this library. I really do not want this library to tie itself to either Commonmark or Jetbrains' Markdown.
  • We can pivot richtext-commonmark to be a solely parser module. It should just provide a way to convert String into an ASTNode defined by richtext-markdown.
  • Additionally anyone can implement their own parser logic using their own dependencies.

The only difficult element in this path is figuring out the API for ASTNode. A couple of options off the top of my head

  • Just make the current ASTNode public since the library is far from 1.0.0, then iterate.
  • Copy the ASTNode API of intellij-markdown.

@halilozercan
Copy link
Owner

#129 adds a similar capability without exposing the commonmark dependency.

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.

3 participants