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

Remove mustache.to_html() #735

Merged
merged 1 commit into from
Jan 15, 2020
Merged

Remove mustache.to_html() #735

merged 1 commit into from
Jan 15, 2020

Conversation

phillipj
Copy link
Collaborator

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.

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.
@phillipj phillipj merged commit f3012a2 into janl:master Jan 15, 2020
@phillipj phillipj deleted the remove-to-html branch January 15, 2020 20:45
@vladimyr
Copy link

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 to_html broke consolidate that is:

  • de facto standard way of plugging mustache into express which is I believe quite common use case
  • unfortunately effectively unmaintained so there is almost zero chance required action will happen on their side

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:

  1. lock version on v3 without any hopes of leveraging all the great stuff that came with v4 and will eventually come with newer versions
  2. patch mustache to reimplement to_html inside their codebases to make consolidate work with it again

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 consolidate's codebase.

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 express/consolidate users that they need to stay on v3 due to these changes.

@vladimyr
Copy link

Adding link to consolidate isssue: tj/consolidate.js#331

@phillipj
Copy link
Collaborator Author

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 consolidate | mustache is not compatible. This is obviously part of the reason peerDependencies exists, but I also see the reason why consolidate in practise cannot list all the template engines its compatible with as peer deps since only one of those would be used in a project I guess.

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? 🤞

@vladimyr
Copy link

vladimyr commented Feb 25, 2020

Thanks a bunch for the elaborate report @vladimyr 👍

You are welcome I honestly hope it didn't sound wrong I just wanted to warn everyone about unexpected outcomes.

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.

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...

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 consolidate | mustache is not compatible. This is obviously part of the reason peerDependencies exists, but I also see the reason why consolidate in practise cannot list all the template engines its compatible with as peer deps since only one of those would be used in a project I guess.

Again, well said. They could, however, list all versions they support under ourCustomPeerDependencies field inside package.json having the same semantics and checking those in runtime using semver but that's pretty unrealistic to ask of folks who don't do maintenance tasks either.

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? 🤞

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 👍

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.

2 participants