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

Remove markdown parsing from frontend #1952

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

siddhantk232
Copy link
Contributor

@siddhantk232 siddhantk232 commented Oct 7, 2024

marked was used to parse markdown
for each text node in the browser which was making things very slow.

This commit moves markdown parsing to the backend using the comrak
crate.

Some cleanups may be required that removes the marked.js files
completely from the codebase but it'll happen later once I get current
changes reviewed.

ftd code that was used to test this

-- ftd.text: 

Hello world `hellow` [link](https://google.com/)

\```rust
fn hello () -> {
    

    println!("works?");
    println!("works?");
    println!("works?");
    println!("works?");
    println!("works?");
    println!("works?");



}
\```

-- ftd.code:
lang: rust

#[derive(Debug)]
pub struct SetProperty {
    pub kind: PropertyKind,
    pub value: SetPropertyValue,
    pub element_name: String,
    pub inherited: String,
}

#[derive(Debug)]
pub enum SetPropertyValue {
    Reference(String),
    Value(fastn_js::Value),
    Formula(fastn_js::Formula),
    Clone(String),
}

impl fastn_js::SetPropertyValue {
    pub fn to_js(&self) -> String {
        self.to_js_with_element_name(&None)
    }
}

I couldn't see any visual changes before and after this commit. Attaching screenshots anyway:

(before this commit)
image

(after this commit)
image

[marked](https://github.com/markedjs/marked/) was used to parse markdown
for each text node in the browser which was making things very slow.

This commit moves markdown parsing to the backend using the `comrak`
crate.

Some cleanups may be required that removes the marked.js files
completely from the codebase but it'll happen later once I get this
changes reviewed.
@siddhantk232 siddhantk232 self-assigned this Oct 7, 2024
@siddhantk232 siddhantk232 requested a review from amitu October 7, 2024 12:57
@siddhantk232
Copy link
Contributor Author

I expect the tests to fail but I'm not too worried about that right now. I want either @amitu or @Arpita-Jaiswal (or both) to first review these changes carefully as I'm not too confident about this.

@amitu
Copy link
Contributor

amitu commented Oct 7, 2024

@siddhantk232 how have you tested it so far? Is fastn.com, fifthtry, ide etc working with it?

@amitu
Copy link
Contributor

amitu commented Oct 7, 2024

Can you also create some stress test / benchmark files, eg with 10K ftd.text nodes (with slightly different content; generated using Python) and do a before and after (backend and frontend timing) to see if it indeed made any improvements or is it a performance regression?

@siddhantk232
Copy link
Contributor Author

siddhantk232 commented Oct 8, 2024

@siddhantk232 how have you tested it so far? Is fastn.com, fifthtry, ide etc working with it?

I tested it with the ftd snippet mentioned in the PR description which worked fine. fastn.com does have some visual differences (ftd code is not highlighted and other code is not indented properly with this change). I didn't see any noticeable performance gains either:

fastn 0.4.76 lighthouse report on fastn.com/web-component/ ran locally with fastn --trace serve --offline

image

Same webpage but with current build of fastn that includes this change

image

@amitu

@siddhantk232
Copy link
Contributor Author

I don't think moving the code highlight part (prism.js) to the backend would make this any better. We should probably not go ahead with this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants