-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Remove mustache.to_html() #735
Conversation
In the spirit of keeping the public API of mustache.js as small as possible for maintainence reasons, the undocumented and un-tested `.to_html()` method is removed. The functionality involved, where it can rather invoke an optional function provided with the result of `.render()`, instead of returning it as a string like `.render()` does, is something that using projects very easily can do themselfs -- it does not have to be provided by mustache.js.
As much as it stands true it is not that simple. Removing
Stakes are quite high if you take a look on download numbers, as of writing this comment number of weekly downloads is 1,510,043 I understand your motivation and I don't want to stop the progress but this is a quite controversial decision that doesn't give many benefits and has potential for becoming a serious obstacle for people using mustache as their express rendering engine. Basically, they are left with two choices:
Of course, that will happen after they learn about it because it is not even obvious they'll be affected by major version bump unless they dive deep into Ideally, this should be reverted until it is safe to be removed. If that is not possible at least there should be a note in the readme directing |
Adding link to |
Thanks a bunch for the elaborate report @vladimyr 👍 It's a shame we're also lacking active maintainers in this project, as doing stuff and opening PRs is a bit like shouting into the forest -- very rarely any response. I could of course have waited longer than I did before merging this removal, but I've gotten too used to the silence around here, so didn't expect that do make any difference to be honest. I emphasise with the pain you're getting at, and it's surely less than ideal that using projects would not be warned when their installed versions of Appreciate you opening tj/consolidate.js#332 to fix this issue. Hopefully a maintainer will merge & publish that much needed fix ASAP. Would you let us know when that happens? 🤞 |
You are welcome I honestly hope it didn't sound wrong I just wanted to warn everyone about unexpected outcomes.
What can I say there? I very much feel your pain. Most of the people simply don't care, they take what they need and never give back which goes hand by hand with that wonderful feeling of entitlement everyone is having these days. TBH I don't think waiting would change anything. I'd probably be the first one to ignore it because I'm not actively watching this repo. OTOH I am in fact maintainer of few others but in general, I don't think it will ever become any different. The only thing you might hear one day is an echo or, in a more brighter vision of future, the voice of some fellow adventurer or person lucky enough to be supported by their employer to work on OSS...
Again, well said. They could, however, list all versions they support under ourCustomPeerDependencies field inside
We'll see. I'm not really optimistic but I don't want to put pressure on anyone's back. I'll definitely ping you back once that happens. 👍 Last but not least, thank you for making time to reply to my comment 👍 |
As we're about to push a new major version because of recent internal caching behaviour changes in #731, I want to take the opportunity to do more major-version-work.
In the spirit of keeping the public API of mustache.js as small as possible for maintainence reasons, the undocumented and un-tested
.to_html()
method is removed.The functionality involved, where it can rather invoke an optional function provided with the result of
.render()
, instead of returning it as a string like.render()
does, is something that using projects very easily can do themselfs -- it does not have to be provided by mustache.js.