Consider not using Handlebars.create()

[allmarkedup]

First - great job on this, I use it all the time.

However, one sticking point is always that regardless of the Handlebars instance that is passed in, you always call the .create() method on it. So if I've already registered partials/helpers then they are not copied across to the new, 'promisified' instance.

If this module didn't call the Handlebars.create() method but instead just used the passed-in Handlebars object directly then existing helpers will still be promisified (as they will be re-registered along with the default Handlebars ones) and as an additional bonus I can still register helpers on the instance that I passed in (and they will be promisified accordingly) rather than getting back a different instance to the one I supplied.

Is there a reason for using the .create() method that I'm missing? Will happily open a PR for this (I don't think it will break backwards compatibility with anything) if it's something you'd consider changing.

[nknapp]

The main reason was that I didn't want to modify objects passed in as parameter. If the Handlebars-instance is used in other places as well, the callers may not expect a Promise-result but a string.

Would it help you if this line would register all helpers of the original Handlebars-instance instead of the copy? Maybe add a line to copy all partials as well?

[allmarkedup]

My first thought was that if I'm passing in a Handlebars instance, I would expect that to be modified, otherwise why pass in an instance at all? I'd say that this was the convention for most handlebars libraries.

However... I hadn't considered the fact that using promised-handlebars could actually 'break' other libraries that are not expecting a promise as a return value from the render methods, so it's definitely trickier.

I think the copying solution you suggest would definitely be a good start, because at a minimum if I pass in a Handlebars instance with helpers and partials registered I would expect those still to exist on the new 'promisified' instance.

However maybe in addition a config option could also be exposed to allow people to request that the original instance is modified and not cloned. So then something like the following would be possible (which would certainly be closer to my expectations):

const Handlebars = require('handlebars');
const promisedHandlebars = require('promised-handlebars');

Handlebars.registerHelper('foo', function(){
    return 'foo';
});

promisedHandlebars(Handlebars, {
    clone: false // or modify: true  or similar!
});

Handlebars.registerHelper('bar', function(){
    return 'bar';
});

// Handlebars instance has both foo and bar helpers on it and returns promises.

If you agree and want me to put this together as a PR for you to have a look over just let me know.

[jskrzypek]

@nknapp @allmarkedup I don't know if further expanding the complexity of the options and increasing amount of code this lib needs to maintain is the right way to go; it could be a good idea, but I think @nknapp should make that call.

As an alternative we could make the small change @allmarkedup suggests and just use the passed-in instance, and then change the docs to suggest consumers use this by default to prevent breaking other libraries:

promisedHandlebars(Handlebars.create());
// or
AsyncHandlebars = Handlebars.create();
promisedHandlebars(AsyncHandlebars);

[nknapp]

tl;dr

• Just changing the behavior and updating the docs is not an option for me. Compatibilty should be maintained.
• Copying all helpers/partials (decorators?) from the passed-in instance is fine.
• If the modify-behavior is really required, (please think about whether you really need it), it should be in a separate function. I am still wary of adding such an option (see below), but if there is a use-case that is not covered by copying helpers/partials, I won't oppose it.

---

The long version

I would refrain from making another major version bump...

... so quickly, so changing the behavior and adapt the documentation is not an option for me. (This is a breaking change after all.) In my experience breaking changes can cause a lot of work for users of the library. I don't want to do this too often.

Copy all helpers/partials (decorators?) from the passed-in instance of Handlebars.

Copying helpers seems to be a good idea and it doesn't change the behavior for cases where promised-handlebars is used in the documented way. Actually, when I created this package I wasn't aware that helpers and partials weren't copied by Handlebars.create().

Provide an alternative method to modify an existing instance of Handlebars.

I think an option would be fine if this would be a slight change. But to me, modifying the passed-in instance of Handlebars is more fundamental than that. If this change is really required, it might be another method (with its own api-docs, pointing out the dangers, and possibly with no return value).
The method could be named .morph() or .modify() and could be used like this:

var promisedHandlebars = require('promised-handlebars')
var Handlebars = require('handlebars')
var AsyncHandlebars = Handlebars.create()
promisedHandlebars.morph(AsyncHandlebars)

I'm still wary of this change, because it bears dangers. If Handlebars is used by two completely independent libraries in the dependency-tree and one of them is using the .morph()-function like to modify the root-instance like

var promisedHandlebars = require('promised-handlebars')
var Handlebars = require('handlebars')
promisedHandlebars.morph(Handlebars)

then the other library will probably break.

A few weeks ago, I spent some hours tracing a bug because of montagejs/collections#139. The collections-package was in the dependency tree of my project and it overwrites the Array.prototype.find-method with other semantics than specified in the ES6-spec. Another package in the dependency-tree relied on the default behaviour and thus broke.

Those bugs are very hard to find and I fear introducing such a function might encourage users to introduce one.

[nknapp]

@allmarkedup I'm inviting you as a collaborator. It may be helpful to work on this in a branch in this repository rather than in your fork.

[jskrzypek]

Fwiw, I agree @nknapp

[allmarkedup]

@jskrzypek @nknapp I'm really sorry but I am now away on holiday until the 27th with very limited access to internet so I can't do anything on this before then - I'll pick up the thread again as soon as I'm back however.

[nknapp]

@allmarkedup No problem. There is no rush (from my point of view). Enjoy your vacation.

[nknapp]

@allmarkedup I am closing this issue due to the long idle time. If you still want to work on this feature, I will reopen the issue (and invite your as collaborator again).