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

feat: add support for the Jekyll markdownify filter #621

Closed

Conversation

TomasHubelbauer
Copy link
Contributor

I have selected Marked as the library to use for the MD > HTML conversion and I am using isomorphic-dompurify because normal DOMPurify doesn't work in the test runner (it only works in the browser environment while isomorphic-dompurify is a wrapper that runs on both browser and Node).

Closes #620

I have selected Marked as the library to use for the MD > HTML conversion and I am using isomorphic-dompurify because normal DOMPurify doesn't work in the test runner (it only works in the browser environment while isomorphic-dompurify is a wrapper that runs on both browser and Node).
- `git switch -c your_branch_name` (do this in your fork not the main repo)
- `git add .`
- `git commit -m "feat: Adding my change"`
- `git commit -m "feat: adding my change"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fix this because it is not compatible with the commit lint used in the repository and when I was creating this document I didn't yet know the rules it enforced very well so this slipped in.

@@ -104,7 +105,9 @@
"typescript": "^4.5.3"
},
"dependencies": {
"commander": "^10.0.0"
"commander": "^10.0.0",
"isomorphic-dompurify": "^1.6.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using isomorphic-dompurify over normal dompurify because that doesn't run in Node and was failing in the test runner. isomorphic-dompurify runs in both the browser and Node.

The `stringify` helper always returns a valid string.
"commander": "^10.0.0"
"commander": "^10.0.0",
"isomorphic-dompurify": "^1.6.0",
"marked": "^5.0.4"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose Marked because it is the most popular regularly maintained Markdown to HTML converter NPM package I could find.

src/filters/string.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made sure this remained in lockfile version 1 but the diff is still pretty wild because isomorphic-dompurify brings in jsdom which itself depends on a lot of stuff.

@TomasHubelbauer
Copy link
Contributor Author

TomasHubelbauer commented Jun 4, 2023

There are two problems with this PR:

It doesn't work in older Node versions (as used in the Check workflow):

/home/runner/work/liquidjs/liquidjs/node_modules/isomorphic-dompurify/node_modules/jsdom/lib/jsdom/living/nodes/HTMLInputElement-impl.js:583
    this[filesSymbol] ||= FileList.createImpl(this._globalObject);

    SyntaxError: Unexpected token '||='

JSDOM's minimal Node version is Node 16:
https://github.com/jsdom/jsdom/blob/master/package.json

LiquidJS minimal Node version is Node 14:
https://github.com/harttle/liquidjs/blob/master/package.json

The Check CI is using Node 14:
https://github.com/harttle/liquidjs/blob/master/.github/workflows/check.yml

@harttle Are you open to bumping the minimal Node version to 16?
If not, I will try to find another dependency such that we can still implement this without reducing support.

And it doesn't work when I build and use the built file in the Playground.

Testing with: {{ 'test' | markdownify }}

N is undefined, line:1, col:1
>> 1| {{ 'test' | markdownify }}
      ^
   2| 
ne@http://localhost:4000/js/liquid.browser.min.js:1:5485
he@http://localhost:4000/js/liquid.browser.min.js:1:6069
Xt.prototype.renderTemplates/<@http://localhost:4000/js/liquid.browser.min.js:1:19675
x/e/<@http://localhost:4000/js/liquid.browser.min.js:1:2207
w/</<@http://localhost:4000/js/liquid.browser.min.js:1:9482
x/e/<@http://localhost:4000/js/liquid.browser.min.js:1:2207
i@http://localhost:4000/js/liquid.browser.min.js:1:1104
promise callback*o@http://localhost:4000/js/liquid.browser.min.js:1:1220
a/<@http://localhost:4000/js/liquid.browser.min.js:1:1231
a@http://localhost:4000/js/liquid.browser.min.js:1:1004
w@http://localhost:4000/js/liquid.browser.min.js:1:9331
P.prototype.parseAndRender/</<@http://localhost:4000/js/liquid.browser.min.js:1:73513
x/e/<@http://localhost:4000/js/liquid.browser.min.js:1:2207
a/<@http://localhost:4000/js/liquid.browser.min.js:1:1253
a@http://localhost:4000/js/liquid.browser.min.js:1:1004
P.prototype.parseAndRender@http://localhost:4000/js/liquid.browser.min.js:1:73446
update@http://localhost:4000/js/main.js:121:33
@http://localhost:4000/js/main.js:78:3
@http://localhost:4000/js/main.js:142:2

From markdownify@http://localhost:4000/js/liquid.browser.min.js:1:53040
_r.prototype.render/<@http://localhost:4000/js/liquid.browser.min.js:1:38013
x/e/<@http://localhost:4000/js/liquid.browser.min.js:1:2207
w/</<@http://localhost:4000/js/liquid.browser.min.js:1:9482
x/e/<@http://localhost:4000/js/liquid.browser.min.js:1:2207
a/<@http://localhost:4000/js/liquid.browser.min.js:1:1253
a@http://localhost:4000/js/liquid.browser.min.js:1:1004
w@http://localhost:4000/js/liquid.browser.min.js:1:9331
w/</<@http://localhost:4000/js/liquid.browser.min.js:1:9569
x/e/<@http://localhost:4000/js/liquid.browser.min.js:1:2207
n@http://localhost:4000/js/liquid.browser.min.js:1:1058
promise callback*o@http://localhost:4000/js/liquid.browser.min.js:1:1220
a/<@http://localhost:4000/js/liquid.browser.min.js:1:1231
a@http://localhost:4000/js/liquid.browser.min.js:1:1004
w@http://localhost:4000/js/liquid.browser.min.js:1:9331
w/</<@http://localhost:4000/js/liquid.browser.min.js:1:9569
x/e/<@http://localhost:4000/js/liquid.browser.min.js:1:2207
a/<@http://localhost:4000/js/liquid.browser.min.js:1:1253
a@http://localhost:4000/js/liquid.browser.min.js:1:1004
w@http://localhost:4000/js/liquid.browser.min.js:1:9331
w/</<@http://localhost:4000/js/liquid.browser.min.js:1:9569
x/e/<@http://localhost:4000/js/liquid.browser.min.js:1:2207
a/<@http://localhost:4000/js/liquid.browser.min.js:1:1253
a@http://localhost:4000/js/liquid.browser.min.js:1:1004
w@http://localhost:4000/js/liquid.browser.min.js:1:9331
P.prototype.parseAndRender/</<@http://localhost:4000/js/liquid.browser.min.js:1:73513
x/e/<@http://localhost:4000/js/liquid.browser.min.js:1:2207
a/<@http://localhost:4000/js/liquid.browser.min.js:1:1253
a@http://localhost:4000/js/liquid.browser.min.js:1:1004
P.prototype.parseAndRender@http://localhost:4000/js/liquid.browser.min.js:1:73446
update@http://localhost:4000/js/main.js:121:33
@http://localhost:4000/js/main.js:78:3
@http://localhost:4000/js/main.js:142:2

@harttle Is there a way to do a non-minimized build so that I could check what the error is? My browser has not picked up the source map.

@@ -4,6 +4,8 @@
* * prefer stringify() to String() since `undefined`, `null` should eval ''
*/
import { assert, escapeRegExp, stringify } from '../util'
import { marked } from 'marked'
import { sanitize } from 'isomorphic-dompurify'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently liquidjs has only one run time dependency commander and that is not included in dist files. Let's avoid introducing dependencies.

Markdown functionality is better to be maintained in another repo, maybe liquidjs-plugin-markdown, or liquidjs-plugin-jekyll.

@harttle
Copy link
Owner

harttle commented Jun 4, 2023

@TomasHubelbauer There is a non minified build under dist/ after npm run build.

@TomasHubelbauer
Copy link
Contributor Author

Closing the PR for now and will redo as a LiquidJS filter once clearer on how to do that.

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 the markdownify filter
2 participants