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

Load markdown parser externally #17

Merged
merged 2 commits into from
Aug 18, 2022
Merged

Load markdown parser externally #17

merged 2 commits into from
Aug 18, 2022

Conversation

powmedia
Copy link
Contributor

No description provided.

@powmedia
Copy link
Contributor Author

@caub this is how I think we can keep the markdown as an easy dependency

@powmedia powmedia merged commit 953aed9 into master Aug 18, 2022
@powmedia powmedia deleted the externalMarkdown branch August 18, 2022 12:27
@caub
Copy link
Contributor

caub commented Aug 18, 2022

ok, note it's good to use peerDependencies to tell "this lib is necessary, but installed apart"

not mandatory, but good practice

ok thanks

@caub
Copy link
Contributor

caub commented Aug 18, 2022

oh actually no, in the way you've done it's completely independant, no peerDependencies

peerDependencies would be if we import it from wurd-web but not bundle it, it's a little bit simpler (no need to pass it in .connect()), but that's fine that way too

@caub
Copy link
Contributor

caub commented Aug 18, 2022

Well, I prefer the peerDependencies approach #7

Your approach is ideal if we wanted to support different markdown libs, but here, specificially on the web, we will always be using marked (markdown-it is too heavy on browser), and having to always pass marked in options make it a bit more annoying to set up, (in some ways we're calling .connect() twice, for loading using cache and then with user's language

Other thing, can we make startEditor() remove a previous script if any? that would help to change the language without having to reload (Edit: sorry this is only useful in ?edit mode), for this user language issue, we will reset the cache, it should work

@powmedia
Copy link
Contributor Author

powmedia commented Aug 19, 2022 via email

@powmedia
Copy link
Contributor Author

powmedia commented Oct 11, 2022 via email

@caub
Copy link
Contributor

caub commented Oct 11, 2022

Yea, in the end I like this approach, no extra weight

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.

None yet

2 participants