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

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

Merged
merged 8 commits into from
Sep 23, 2024

Conversation

jasonnathan
Copy link
Contributor

  • Introduced langConfig option to allow custom language grammars and aliases.
  • Updated test cases to validate custom language support.

- Introduced langConfig option to allow custom language grammars and aliases.
- Updated test cases to validate custom language support.
@rossrobino rossrobino linked an issue Sep 22, 2024 that may be closed by this pull request
@rossrobino
Copy link
Owner

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
Copy link
Owner

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

@jasonnathan
Copy link
Contributor Author

jasonnathan commented Sep 23, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

jasonnathan commented Sep 23, 2024

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
Copy link
Owner

rossrobino commented Sep 23, 2024

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.

@rossrobino rossrobino merged commit e28349e into rossrobino:main Sep 23, 2024
@jasonnathan
Copy link
Contributor Author

Hey no worries, thanks for the packages :)

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.

Add Support for Custom Language Grammars in processMarkdown
2 participants