From dbcb905bb37d6fa9675099a42b2a6c90f19b6ed7 Mon Sep 17 00:00:00 2001 From: ole-martin <47389666+ole-martin@users.noreply.github.com> Date: Fri, 8 Nov 2019 13:46:34 +0100 Subject: [PATCH 1/8] fix(runtime.js): partials compile not caching 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 https://github.com/wycats/handlebars.js/issues/1598 --- lib/handlebars/runtime.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/handlebars/runtime.js b/lib/handlebars/runtime.js index 958afc3ef..dec00da2e 100644 --- a/lib/handlebars/runtime.js +++ b/lib/handlebars/runtime.js @@ -161,7 +161,11 @@ export function template(templateSpec, env) { container.helpers = Utils.extend({}, env.helpers, options.helpers); if (templateSpec.usePartial) { - container.partials = Utils.extend({}, env.partials, options.partials); + if (!options.partials || !env.partials) { + container.partials = options.partials || env.partials; + } else { + container.partials = Utils.extend({}, env.partials, options.partials); + } } if (templateSpec.usePartial || templateSpec.useDecorators) { container.decorators = Utils.extend({}, env.decorators, options.decorators); From 9051668f152f34e400b7da436f64fea6e77a93a7 Mon Sep 17 00:00:00 2001 From: ole-martin <47389666+ole-martin@users.noreply.github.com> Date: Mon, 11 Nov 2019 09:24:29 +0100 Subject: [PATCH 2/8] Add compilation count test for global partials --- spec/compiler.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/spec/compiler.js b/spec/compiler.js index 02dad6751..c8b734764 100644 --- a/spec/compiler.js +++ b/spec/compiler.js @@ -108,4 +108,29 @@ describe('compiler', function() { equal(/return ""/.test(Handlebars.precompile('')), true); }); }); + + describe('Compile Partials Correctly', function() { + var compileCount = 0; + var origTemplateFunc = Handlebars.template; + before(function() { + Handlebars.template = function(...params) { + compileCount++; + return origTemplateFunc(...params); + }; + Handlebars.registerPartial({ + 'dude': 'I am a partial' + }); + }); + after(function() { + Handlebars.template = origTemplateFunc; + // Handlebars.unregisterPartial('dude'); + }); + + it('should only compile global partials once', function() { + var string = 'Dudes: {{> dude}} {{> dude}}'; + Handlebars.compile(string)(); // This should compile template + partial once + Handlebars.compile(string)(); // This should only compile template + equal(compileCount, 3); + }); + }); }); From f9a0997e8a56a4159827c9d12e194ea0934e0595 Mon Sep 17 00:00:00 2001 From: ole-martin <47389666+ole-martin@users.noreply.github.com> Date: Mon, 11 Nov 2019 09:27:20 +0100 Subject: [PATCH 3/8] Reintroduce old merge function for partials --- lib/handlebars/runtime.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/handlebars/runtime.js b/lib/handlebars/runtime.js index dec00da2e..251900370 100644 --- a/lib/handlebars/runtime.js +++ b/lib/handlebars/runtime.js @@ -124,6 +124,15 @@ export function template(templateSpec, env) { } return value; }, + merge: function(param, common) { + let obj = param || common; + + if (param && common && (param !== common)) { + obj = Utils.extend({}, common, param); + } + + return obj; + }, // An empty object to use as replacement for null-contexts nullContext: Object.seal({}), @@ -160,12 +169,9 @@ export function template(templateSpec, env) { if (!options.partial) { container.helpers = Utils.extend({}, env.helpers, options.helpers); - if (templateSpec.usePartial) { - if (!options.partials || !env.partials) { - container.partials = options.partials || env.partials; - } else { - container.partials = Utils.extend({}, env.partials, options.partials); - } + if (templateSpec.usePartial) { + // Use merge here to prevent compiling global partials multiple times + container.partials = container.merge(options.partials, env.partials); } if (templateSpec.usePartial || templateSpec.useDecorators) { container.decorators = Utils.extend({}, env.decorators, options.decorators); From 94b42286d8077f45d7a04beb0260fca6ac1c83db Mon Sep 17 00:00:00 2001 From: ole-martin <47389666+ole-martin@users.noreply.github.com> Date: Mon, 11 Nov 2019 09:31:00 +0100 Subject: [PATCH 4/8] Fix trailing spaces --- lib/handlebars/runtime.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/handlebars/runtime.js b/lib/handlebars/runtime.js index 251900370..5419fbb8c 100644 --- a/lib/handlebars/runtime.js +++ b/lib/handlebars/runtime.js @@ -169,7 +169,7 @@ export function template(templateSpec, env) { if (!options.partial) { container.helpers = Utils.extend({}, env.helpers, options.helpers); - if (templateSpec.usePartial) { + if (templateSpec.usePartial) { // Use merge here to prevent compiling global partials multiple times container.partials = container.merge(options.partials, env.partials); } From e7c097cf48cdf6ad4a370482f0b09a2dda412d29 Mon Sep 17 00:00:00 2001 From: ole-martin <47389666+ole-martin@users.noreply.github.com> Date: Mon, 11 Nov 2019 09:32:57 +0100 Subject: [PATCH 5/8] Fix linting --- spec/compiler.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/compiler.js b/spec/compiler.js index c8b734764..e2ee32f64 100644 --- a/spec/compiler.js +++ b/spec/compiler.js @@ -108,14 +108,14 @@ describe('compiler', function() { equal(/return ""/.test(Handlebars.precompile('')), true); }); }); - + describe('Compile Partials Correctly', function() { var compileCount = 0; var origTemplateFunc = Handlebars.template; before(function() { - Handlebars.template = function(...params) { + Handlebars.template = function(params) { compileCount++; - return origTemplateFunc(...params); + return origTemplateFunc(params); }; Handlebars.registerPartial({ 'dude': 'I am a partial' @@ -123,7 +123,7 @@ describe('compiler', function() { }); after(function() { Handlebars.template = origTemplateFunc; - // Handlebars.unregisterPartial('dude'); + Handlebars.unregisterPartial('dude'); }); it('should only compile global partials once', function() { From d5e932060027c41f133561b2add961842b93009f Mon Sep 17 00:00:00 2001 From: ole-martin <47389666+ole-martin@users.noreply.github.com> Date: Tue, 12 Nov 2019 08:22:30 +0100 Subject: [PATCH 6/8] Rename merge to mergeIfNeeded --- lib/handlebars/runtime.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/handlebars/runtime.js b/lib/handlebars/runtime.js index 5419fbb8c..59b1cc106 100644 --- a/lib/handlebars/runtime.js +++ b/lib/handlebars/runtime.js @@ -124,7 +124,7 @@ export function template(templateSpec, env) { } return value; }, - merge: function(param, common) { + mergeIfNeeded: function(param, common) { let obj = param || common; if (param && common && (param !== common)) { @@ -170,8 +170,8 @@ export function template(templateSpec, env) { container.helpers = Utils.extend({}, env.helpers, options.helpers); if (templateSpec.usePartial) { - // Use merge here to prevent compiling global partials multiple times - container.partials = container.merge(options.partials, env.partials); + // Use mergeIfNeeded here to prevent compiling global partials multiple times + container.partials = container.mergeIfNeeded(options.partials, env.partials); } if (templateSpec.usePartial || templateSpec.useDecorators) { container.decorators = Utils.extend({}, env.decorators, options.decorators); From 8082488bf6ebe422961c2438d873eaf276806aaa Mon Sep 17 00:00:00 2001 From: Ole-Martin Date: Mon, 18 Nov 2019 10:31:08 +0100 Subject: [PATCH 7/8] refactor: move test to regression and use sinon.spy --- spec/compiler.js | 25 ------------------------- spec/regressions.js | 27 +++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/spec/compiler.js b/spec/compiler.js index 7b9350966..3d37b5940 100644 --- a/spec/compiler.js +++ b/spec/compiler.js @@ -108,29 +108,4 @@ describe('compiler', function() { equal(/return ""/.test(Handlebars.precompile('')), true); }); }); - - describe('Compile Partials Correctly', function() { - var compileCount = 0; - var origTemplateFunc = Handlebars.template; - before(function() { - Handlebars.template = function(params) { - compileCount++; - return origTemplateFunc(params); - }; - Handlebars.registerPartial({ - 'dude': 'I am a partial' - }); - }); - after(function() { - Handlebars.template = origTemplateFunc; - Handlebars.unregisterPartial('dude'); - }); - - it('should only compile global partials once', function() { - var string = 'Dudes: {{> dude}} {{> dude}}'; - Handlebars.compile(string)(); // This should compile template + partial once - Handlebars.compile(string)(); // This should only compile template - equal(compileCount, 3); - }); - }); }); diff --git a/spec/regressions.js b/spec/regressions.js index 74594de6b..8b6a1629b 100644 --- a/spec/regressions.js +++ b/spec/regressions.js @@ -334,4 +334,31 @@ describe('Regressions', function() { shouldCompileTo('{{helpa length="foo"}}', [obj, helpers], 'foo'); }); + + describe('GH-1598: Performance degradation for partials since v4.3.0', function() { + // Do not run test for runs without compiler + if (!Handlebars.compile) { + return; + } + + var newHandlebarsInstance; + before(function() { + newHandlebarsInstance = Handlebars.create(); + }); + after(function() { + sinon.restore(); + }); + + it('should only compile global partials once', function() { + var templateSpy = sinon.spy(newHandlebarsInstance, 'template'); + newHandlebarsInstance.registerPartial({ + 'dude': 'I am a partial' + }); + var string = 'Dudes: {{> dude}} {{> dude}}'; + newHandlebarsInstance.compile(string)(); // This should compile template + partial once + newHandlebarsInstance.compile(string)(); // This should only compile template + equal(templateSpy.callCount, 3); + sinon.restore(); + }); + }); }); From a65bd6a8d549c3ae649fb8b50114188433ca286f Mon Sep 17 00:00:00 2001 From: Nils Knappmeier Date: Mon, 18 Nov 2019 20:41:15 +0100 Subject: [PATCH 8/8] change "before/after" to "beforeEach/afterEach" --- spec/regressions.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/regressions.js b/spec/regressions.js index 8b6a1629b..3eccb2817 100644 --- a/spec/regressions.js +++ b/spec/regressions.js @@ -342,10 +342,10 @@ describe('Regressions', function() { } var newHandlebarsInstance; - before(function() { + beforeEach(function() { newHandlebarsInstance = Handlebars.create(); }); - after(function() { + afterEach(function() { sinon.restore(); });