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

Provide access to 'smartypantsu' somehow #168

Closed
webketje opened this issue Feb 1, 2024 · 5 comments
Closed

Provide access to 'smartypantsu' somehow #168

webketje opened this issue Feb 1, 2024 · 5 comments

Comments

@webketje
Copy link
Contributor

webketje commented Feb 1, 2024

In the marked implementation here: https://github.com/alphagov/govuk-design-system/blob/main/lib/marked/extension.js#L16-L25,
it became clear to me that users opting out of HTML entity encoding are forced to npm install smartypants separately to have backwards-compatible processing with pre-marked 5.0.0 smartypants option (and as in marked-smartypants-lite).

Perhaps add an option next to config: htmlEntities (default=true) - "HTML-encode special characters", or utf8 (default=false) "Do not HTML-encode special characters"?

Willing to contribute a PR when the direction is clear

@UziTech
Copy link
Member

UziTech commented Feb 1, 2024

I think a better option would be to add config option to smartypants to output html entities or utf8. That way config is all in one place

@webketje
Copy link
Contributor Author

webketje commented Feb 2, 2024

True, but having the option is stil better than not having it and at least marked-smartypants can have all its config in one place + such a small effort.

For the users and depending packages of the most popular markdown js package that didn't have the time to upgrade 6 major versions in half a year and who relied on the smartypants option pre-5.x that was substituted for a similarly-named package with a silent but very impactful breaking change (multiplying output size and considerably degrading render perf cf #33).
Though I find the quality of its code dubious, I take it there was merit in depending on the smartypants package?

If anything the single method used should be smartypantsu, because why add kilobytes to your page when HTML entities are no longer required to render most UTF-8 chars since at least 2009? https://stackoverflow.com/questions/520236/should-i-still-use-html-entities-why. But with this smartypants dependency, smartypantsu function actually adds even more overhead instead of less 🤦‍♂️

@UziTech
Copy link
Member

UziTech commented Feb 2, 2024

but having the option is stil better than not having it

Users still have the option to do it the same way you did in postprocess.

I would argue having a simple options for everything is not always better than not having it
image

I take it there was merit in depending on the smartypants package?

Just that it is the only npm package that matches the full smartypants spec. If you find (or create) a different package I have no problems with switching to that.


Honestly I think it would be easier and use less bites to not use smartypants

https://github.com/othree/smartypants.js#why-you-might-not-want-to-use-smart-quotes-in-your-weblog

@webketje
Copy link
Contributor Author

webketje commented Feb 2, 2024

I would argue having a simple options for everything is not always better than not having it
Honestly I think it would be easier and use less bites to not use smartypants

Totally agree and don't even use it personally. I'm coming at this from a point of view of minimizing breaking changes and user turnover as a major package maintainer. I commend the move to extensions but conclude that I am forced to include marked-smartypants-lite by default instead of marked-smartypants while upgrading @metalsmith/markdown to the latest marked, and adding a warning in the README discouraging use of this package until the issues with its dependency are adressed.

@webketje webketje closed this as completed Feb 2, 2024
@webketje webketje closed this as not planned Won't fix, can't repro, duplicate, stale Feb 2, 2024
@UziTech
Copy link
Member

UziTech commented Feb 2, 2024

If you don't want breaking changes than marked-smartypants-lite is the right way to go. That extension models the way marked did smartypants pre v5. It does a not follow the spec at all which is why we have two packages. Users can choose spec compliant or light 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

No branches or pull requests

2 participants