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 the markdownify filter #620

Open
TomasHubelbauer opened this issue Jun 4, 2023 · 7 comments
Open

Add support for the markdownify filter #620

TomasHubelbauer opened this issue Jun 4, 2023 · 7 comments

Comments

@TomasHubelbauer
Copy link
Contributor

TomasHubelbauer commented Jun 4, 2023

Hi!

In addition to the push and sample Jekyll filters I've added recently, I am looking at adding markdownify (convert Markdown to HTML) to make the Playground usable for my scripts which also use this filter.

This is a string to string filter, so it would go to https://github.com/harttle/liquidjs/blob/master/src/filters/string.ts.

My idea is to pull in an NPM package for converting MarkDown to HTML. I see the library is so far dependency-free except for commander and I am not sure if you would be okay with adding a dependency for is. If not, let me know.

But if you would be, I've looked at the most popular ones here:
https://www.npmjs.com/search?ranking=popularity&q=markdown

The top result has a last release 10 years ago so I am thinking of pulling in marked.
Additionally, Marked recommend to run the output HTML through an HTML sanitizer library to stop any sort of XSS attacks and the like.
They recommend https://github.com/cure53/DOMPurify specifically so I would use that as well.

Overall the filter should look roughly something like this:

export function markdownify (v: string) {
  return DOMPurify.sanitize(marked.parse(stringify(v)));
}

I will add the XSS case as one of the test cases to make sure the sanitisation works.

I am working on this ATM but please LMK if you see any issues in the meanwhile and I will address everything in the PR once I am ready to publish it!

@TomasHubelbauer
Copy link
Contributor Author

I've opened a PR for this, but it has some issues. I have left PR comments on it.

@harttle
Copy link
Owner

harttle commented Jun 4, 2023

As we're discussing on that PR, I prefer markdown functionalities not built in LiquidJS. Currently LiquidJS is a small lib without dependencies bundled in. Markdown is a whole system of parsers and renders which also has a significant bundle size impact.

I'm thinking maybe it's better to create a liquidjs-plugin-jekyll to contain all the filters that're too big for LiquidJS and those Jekyll specific.

@TomasHubelbauer
Copy link
Contributor Author

That sounds like a good idea! Is there a plugin system already or would we be introducing one for these more complex and bigger filters?

@harttle
Copy link
Owner

harttle commented Jun 4, 2023

There is one, try this: https://liquidjs.com/tutorials/plugins.html

The concerning part is we've got to maintain it once there's breaking changes on LiquidJS, and plugins tend to depend a lot on LiquidJS codebase.

@harttle
Copy link
Owner

harttle commented Jun 4, 2023

There is one, try this: https://liquidjs.com/tutorials/plugins.html

The concerning part is we got to maintain it once there's breaking changes on LiquidJS, and plugins tend to depend a lot on LiquidJS codebase.

@TomasHubelbauer
Copy link
Contributor Author

I will try to write the plugin and I will keep it in my personal account if you want and put a disclaimer about the status of maintenance. I need it for me right now so I will develop it and put it up and try to maintain it, but I make no promises and if it breaks and someone else needs it working, I will invite them to contribute fixes. Since it will be in my personal account, it will be apparent that it is not an official LiquidJS plugin.

But I would like this plugin to work in the Playground so if you don't mind I would try to extend the playground to allow loading plugins as well. I use the Playground a lot and the contributions I have made so far were motivated by adding missing pieces I needed to the Playground.

@harttle
Copy link
Owner

harttle commented Jun 6, 2023

I would like this plugin to work in the Playground so if you don't mind

That's a good idea. And I'm happy to incorporate e2e test for plugins to ensure they're not broken.

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

Successfully merging a pull request may close this issue.

2 participants