bent10 / marked-extensions

Marked extensions workspace
https://www.npmjs.com/search?q=keywords:stilearning-marked-extensions
MIT License
33 stars 6 forks source link

marked-plaintify port to Marked v13 #101

Closed irian-codes closed 3 months ago

irian-codes commented 4 months ago

Hello! πŸ‘‹

I'm using your library in a personal project,and I noticed it was crashing on Marked latest v13 version. So I decided to update it so it works well. It's helping me parse Markdown into plain text so I can embed it into a vector database for LLMs to retrieve it, so thank you for your work!

Seems Marked pushed breaking changes with the v13 release and I tried to adapt it. Tests pass, for all packages (the only one failing was plaintify) so it seems good.

I had to introduce a breaking change, which is passing the Marked instance to the function. This seems necessary because the new way they're doing it calls the Parser instance (old way) vs (new way). However, if you know any way this breaking change could be avoided let me know!

There's also one Typescript error in some lines but we can know by the code that it's not an issue. Let me know if you want to just ignore it with @ts-ignore or find a better way so TS is happy.

This is my first open source contribution so sorry if I made anything wrong πŸ™

Have a great day!

bent10 commented 3 months ago

Hello! πŸ‘‹

Thank you for updating the library to work with Marked v13 and for your detailed explanation. It's great to hear our library is helping with your project!

I'll review the changes in detail to see if there's a way to avoid this breaking change, but your solution seems solid for now.

Regarding the TypeScript error, let's go ahead and use @ts-ignore for now. We can revisit it later to find a more permanent fix.

Thank you again for your contribution! It's always exciting to see new contributors in open source. You did a great job, and I look forward to integrating your changes. 😊

irian-codes commented 3 months ago

Thank you for your words!

I'm glad my solution seems solid to you. I've made a new commit adding the error comments so you can read and review them whenever you can. It’s been great contributing to this project, and I’m excited to see it integrated.

Thanks for the library, it's very useful 😊

irian-codes commented 3 months ago

Of course! Using the function syntax uses the this value from the caller, great approach! I've run the tests too and it works.

Furthermore, since we're already setting this correctly now, I think we can revert all the plainTextRenderer.[function]?.call(this, args) to the previous ones so plainTextRenderer.tablecell?.call(this, token.header[j]) becomes this.tablecell(token.header[j]), right?

Check out this code. It passes all tests and has zero Typescript errors 😁

BTW sorry for the delay, I had to attend family these days and I wasn't available.

bent10 commented 3 months ago

That’s fantastic! You can push your code for merging.

However, if you don’t mind, I recommend creating a new pull request to avoid the BREAKING CHANGE: token on commit b3f11f9. Otherwise, it will trigger semantic-release to create a major version release when CI runs.

BTW sorry for the delay, I had to attend family these days and I wasn't available.

No worries, this is an open source project πŸ˜ƒ

irian-codes commented 3 months ago

Closing in favour of PR 104 which doesn't include breaking changes so CI doesn't get triggered.