rossrobino / robino

JS modules
MIT License
1 stars 1 forks source link

Add Support for Custom Language Grammars in processMarkdown (Fixes #11) #12

Closed jasonnathan closed 1 month ago

jasonnathan commented 1 month ago
rossrobino commented 1 month ago

Let me know what you think of this. I refactored it into a class. One thing I ran into was that every time processMarkdown was called, it would call mdIt.use again. This way the user creates the processor with their options once, and then they can call processor.process to process markdown with their options.

I also excluded any default languages and made highlight.langs required in the constructor to minimize bundle size.

rossrobino commented 1 month ago

Bumping to 1.0 since I know someone else is using it, will follow semantic versioning.

jasonnathan commented 1 month ago

I actually preferred the function as opposed to a class. It made it easier to compose. Now it has to be instantiated before usage. However, I do understand that the options object became pretty verbose.

One way is a higher order function: createProcessMarkdown(options: Options) => processMarkdown({md}: {md:string}) because this way processMarkdown is preserved as is and both can be exported?

top of my head...

const createProcessMarkdown = (options: Options) => {
   /**
   * perform all merging logic here so the final function is cleaner
   *  const merged = {...defaultOptions, ..options}
   **/
   return ({md}) => processMarkdown({...merged, md})
}
rossrobino commented 1 month ago

Yeah agreed, it is a bit more verbose with the class. So in that case, where would the markdownIt and the highlighter be created? I was having a hard time not calling mdIt.use multiple times on the same MardownIt instance if I put that into the process function and process multiple strings.

I mainly use this during SSR, so I usually need to call it multiple times in different routes after I instantiate it. I'd like to avoid having to create the markdownIt and the highlighter on every call if that makes sense.

jasonnathan commented 1 month ago

Well if you'd like to preserve the way it worked before, then you can call mdIt.use outside the processMarkdown just as it was before. The higher order createProcessMarkdown will call it again but this way the functionality is encapsulated within the higher order, and we'd not have to repeatedly call on mdIt.use on every invocation. I'll update the PR to illustrate. Give me a few minutes :)

jasonnathan commented 1 month ago

Hi there,

I've added two new files, fp.ts and fp.test.ts, to illustrate my approach. I really liked the structure of MarkdownProcessorOptions in your implementation, as it feels much cleaner. However, it also requires users to have a deeper understanding of shiki/core, which might make it a bit more complex for the average user.

In my version, I focused on preserving the functionality of processMarkdown by introducing a higher-order function. This allows users to customise the MarkdownIt and Shiki configurations while keeping the default processMarkdown function intact. This way, existing code (e.g. in typo) doesn’t need to be updated, but users can still leverage the new customisation options when needed.

The current implementation is not final – for instance, it always includes the default languages (like Bash and HTML) even if they're not needed, but I wanted to give you a better idea of the direction I'm heading with this.

Let me know your thoughts, and how you'd like to proceed with refining this further.

Cheers!

rossrobino commented 1 month ago

Hey I really appreciate you showing me this, learned something new about functional programming. I made a few edits to the class based on your new version.

Since it's likely not many people use this package outside of me and you, I think I am going to error on the side of having it be a bit harder to configure but a smaller bundle size by not having the defaults for languages. The size of each language is quite substantial so I think it's better to only include the ones you need. Thanks for making this PR, I think it will be a lot better to have this be configurable now.

I still think even with the class and no defaults, it's not too bad to configure.

import { MarkdownProcessor } from "@robino/md";
import langHtml from "shiki/langs/html.mjs";
import langLua from "shiki/langs/lua.mjs";
import langMd from "shiki/langs/md.mjs";
import langTsx from "shiki/langs/tsx.mjs";

const md = // ...

const processor = new MarkdownProcessor({
    highlighter: {
        langs: [langHtml, langMd, langTsx, langLua],
        langAlias: {
            ts: "tsx",
        },
    },
});

const data = processor.process(md);

Also it speeds up the import since we are not running any code at the top level by using the constructor instead. Then the user can decide when they want to instantiate the code.

Thanks again, I hope this suits your needs.

jasonnathan commented 1 month ago

Hey no worries, thanks for the packages :)