fix(runtime.js): partials compile not caching

[ole-martin]

Reintroduce assigning env.partials to container.partials if no options.partials is set. Also allow assigning options.partials to container.partials if only that one exists. This solves #1598

[andersk]

Perhaps helpers and decorators should also go back to using merge like they did before 2078c72?

[ole-martin]

@andersk Afaik they aren't compiled, so there won't be a big difference like with partials, but it will potentially have some speed-up as it will skip the shallow-copy step if not needed. @nknapp what do you think?

[nknapp]

I think the overhead of the shallow-copy step is negligible.

#1594 is also related to that place. It makes sure that the helpers-object has a "null"-prototype, because some exploits have used "defineGetter" to do bad things and the "helpers"-object is accessible from the template.

Merging here makes sure that the "helpers"-option is not used "as is" and together with #1594 it ensure the null-prototype (Object.create(null)) for this object, so: No, I would like to leave helpers and decorators as they are.

@ole-martin could you rename the merge-function to something that explains what the function is doing. It's not a simple "merge", but a "merge if needed" or a "first non-null or merge" or something like that. If you find a good name, short yet describing what the function does, please take it. But "firstNonNullOrMerged" would be fine to.

I was confused the first time I read that code and my intention (over time) is to make the Handlebars code more explanatory and clean (not by comments, but by good naming).

[ole-martin]

@nknapp Updated function name to "mergeIfNeeded" now 👍

[nknapp]

@ole-martin forgive me for delaying this further again. Although I had added some change requests, I haven't had time for a full review yet. And now it is getting too late again. But I will look...

[nknapp]

I have added some change-requests.

Have you made sure that the test fails without you patch?

I will merge the PR without the sinon-changes if you don't want to do that. But it would be nicer that way.

Update: I hope you are not that fast with starting the rebase: I have just added another commit to the 4.x-branch, because "sinon" was not defined as global variable in the eslint-configuration. It now is.

[ole-martin]

@nknapp I've moved the test to regressions.js and changed it to use sinon-spy & Handlebars.create(). I've also verified that without the changes to runtime the test fails with

1) Regressions
       GH-1598: Performance degradation for partials since v4.3.0
         should only compile global partials once:
     Error: '4' should === '3'

[nknapp]

Great, I'll have another look this evening. But I think its fine now. Thanks.