From 7737eb4a86dccf5b1f228cbf761b48a0e9c8258b Mon Sep 17 00:00:00 2001 From: Sergio Arbeo Date: Tue, 28 Jun 2016 00:23:23 +0200 Subject: [PATCH] [BUGFIX beta] Keep rest positional parameters when nesting contextual components if needed. Steps to reproduce the bug before this patch: 1. Create a component with rest positional param (like `link-to`). 2. Create contextual component (`(component "link-to" "index")`). 3. Render it (`{{component (component "link-to" "index")}}`). 4. Realize the component did not receive `"index"` at all. What was causing it? ==================== Because of how positional parameters work (both in components and contextual components), it is needed to treat them at each level and then merge them. Sadly, this was causing to overwrite the rest positional parameters because when no parameters are passed, the attribute get an empty array nonetheless. In the example above, assuming `LinkToComponent.positionalParams === 'params'`: 1. Contextual component in `2` gets its positional parameters processed and receives the following named parameters `{ params: ['index'] }`. 2. When rendering, the `{{component}}` processes its own parameters getting `{ params: [] }`. Then it merges in a `new EmptyObject()` these attributes on top of the ones in the contextual component. (See what I did there? I used the JS keyword `new` as an adjective! He he. Don't judge me, it is pretty late in here) Solving the issue ================= This PR changes the implementation of `mergeInNewHash` to keep the original positional parameters if the new ones are empty and the positional parameter is of type rest. Fixes #13742 --- .../components/closure-components-test.js | 136 ++++++++++++++++++ .../ember-htmlbars/lib/hooks/component.js | 8 +- .../lib/keywords/closure-component.js | 56 +++++++- .../lib/keywords/element-component.js | 8 +- .../lib/utils/extract-positional-params.js | 6 +- 5 files changed, 205 insertions(+), 9 deletions(-) diff --git a/packages/ember-glimmer/tests/integration/components/closure-components-test.js b/packages/ember-glimmer/tests/integration/components/closure-components-test.js index 2f82fc325c7..43bd7b5cdb7 100644 --- a/packages/ember-glimmer/tests/integration/components/closure-components-test.js +++ b/packages/ember-glimmer/tests/integration/components/closure-components-test.js @@ -40,6 +40,142 @@ moduleFor('@htmlbars Components test: closure components', class extends Renderi this.assertText('Hodi Hodari'); } + ['@test GH#13742 keeps nested rest positional parameters if rendered with no positional parameters']() { + this.registerComponent('-looked-up', { + ComponentClass: Component.extend().reopenClass({ + positionalParams: 'params' + }), + template: '{{#each params as |p|}}{{p}}{{/each}}' + }); + + this.render('{{component (component "-looked-up" model.greeting model.name)}}', { + model: { + greeting: 'Gabon ', + name: 'Zack' + } + }); + + this.assertText('Gabon Zack'); + + this.runTask(() => this.rerender()); + + this.assertText('Gabon Zack'); + + this.runTask(() => this.context.set('model.greeting', 'Good morning ')); + + this.assertText('Good morning Zack'); + + this.runTask(() => this.context.set('model.name', 'Matthew')); + + this.assertText('Good morning Matthew'); + + this.runTask(() => this.context.set('model', { greeting: 'Gabon ', name: 'Zack' })); + + this.assertText('Gabon Zack'); + } + + ['@test overwrites nested rest positional parameters if rendered with positional parameters']() { + this.registerComponent('-looked-up', { + ComponentClass: Component.extend().reopenClass({ + positionalParams: 'params' + }), + template: '{{#each params as |p|}}{{p}}{{/each}}' + }); + + this.render('{{component (component "-looked-up" model.greeting model.name) model.name model.greeting}}', { + model: { + greeting: 'Gabon ', + name: 'Zack' + } + }); + + this.assertText('ZackGabon '); + + this.runTask(() => this.rerender()); + + this.assertText('ZackGabon '); + + this.runTask(() => this.context.set('model.greeting', 'Good morning ')); + + this.assertText('ZackGood morning '); + + this.runTask(() => this.context.set('model.name', 'Matthew')); + + this.assertText('MatthewGood morning '); + + this.runTask(() => this.context.set('model', { greeting: 'Gabon ', name: 'Zack' })); + + this.assertText('ZackGabon '); + } + + ['@test GH#13742 keeps nested rest positional parameters if nested and rendered with no positional parameters']() { + this.registerComponent('-looked-up', { + ComponentClass: Component.extend().reopenClass({ + positionalParams: 'params' + }), + template: '{{#each params as |p|}}{{p}}{{/each}}' + }); + + this.render('{{component (component (component "-looked-up" model.greeting model.name))}}', { + model: { + greeting: 'Gabon ', + name: 'Zack' + } + }); + + this.assertText('Gabon Zack'); + + this.runTask(() => this.rerender()); + + this.assertText('Gabon Zack'); + + this.runTask(() => this.context.set('model.greeting', 'Good morning ')); + + this.assertText('Good morning Zack'); + + this.runTask(() => this.context.set('model.name', 'Matthew')); + + this.assertText('Good morning Matthew'); + + this.runTask(() => this.context.set('model', { greeting: 'Gabon ', name: 'Zack' })); + + this.assertText('Gabon Zack'); + } + + ['@test overwrites nested rest positional parameters if nested with new pos params and rendered with no positional parameters']() { + this.registerComponent('-looked-up', { + ComponentClass: Component.extend().reopenClass({ + positionalParams: 'params' + }), + template: '{{#each params as |p|}}{{p}}{{/each}}' + }); + + this.render('{{component (component (component "-looked-up" model.greeting model.name) model.name model.greeting)}}', { + model: { + greeting: 'Gabon ', + name: 'Zack' + } + }); + + this.assertText('ZackGabon '); + + this.runTask(() => this.rerender()); + + this.assertText('ZackGabon '); + + this.runTask(() => this.context.set('model.greeting', 'Good morning ')); + + this.assertText('ZackGood morning '); + + this.runTask(() => this.context.set('model.name', 'Matthew')); + + this.assertText('MatthewGood morning '); + + this.runTask(() => this.context.set('model', { greeting: 'Gabon ', name: 'Zack' })); + + this.assertText('ZackGabon '); + } + ['@test renders with component helper with curried params, hash']() { this.registerComponent('-looked-up', { ComponentClass: Component.extend().reopenClass({ diff --git a/packages/ember-htmlbars/lib/hooks/component.js b/packages/ember-htmlbars/lib/hooks/component.js index dbe115b40f2..e7e17e80f90 100644 --- a/packages/ember-htmlbars/lib/hooks/component.js +++ b/packages/ember-htmlbars/lib/hooks/component.js @@ -8,8 +8,9 @@ import { } from 'ember-htmlbars/system/lookup-helper'; import extractPositionalParams from 'ember-htmlbars/utils/extract-positional-params'; import { - COMPONENT_PATH, COMPONENT_HASH, + COMPONENT_PATH, + COMPONENT_POSITIONAL_PARAMS, isComponentCell, mergeInNewHash, processPositionalParamsFromCell, @@ -41,8 +42,11 @@ export default function componentHook(renderNode, env, scope, _tagName, params, */ let newAttrs = assign(new EmptyObject(), attrs); processPositionalParamsFromCell(componentCell, params, newAttrs); + attrs = mergeInNewHash(componentCell[COMPONENT_HASH], + newAttrs, + componentCell[COMPONENT_POSITIONAL_PARAMS], + params); params = []; - attrs = mergeInNewHash(componentCell[COMPONENT_HASH], newAttrs); } } diff --git a/packages/ember-htmlbars/lib/keywords/closure-component.js b/packages/ember-htmlbars/lib/keywords/closure-component.js index 4696ce90ced..3dbd5d19e6c 100644 --- a/packages/ember-htmlbars/lib/keywords/closure-component.js +++ b/packages/ember-htmlbars/lib/keywords/closure-component.js @@ -4,6 +4,7 @@ */ import { assert } from 'ember-metal/debug'; +import isEmpty from 'ember-metal/is_empty'; import isNone from 'ember-metal/is_none'; import symbol from 'ember-metal/symbol'; import BasicStream from '../streams/stream'; @@ -11,7 +12,7 @@ import EmptyObject from 'ember-metal/empty_object'; import { read } from '../streams/utils'; import { labelForSubexpr } from 'ember-htmlbars/hooks/subexpr'; import assign from 'ember-metal/assign'; -import { processPositionalParams } from 'ember-htmlbars/utils/extract-positional-params'; +import { isRestPositionalParams, processPositionalParams } from 'ember-htmlbars/utils/extract-positional-params'; import lookupComponent from 'ember-views/utils/lookup-component'; export const COMPONENT_REFERENCE = symbol('COMPONENT_REFERENCE'); @@ -82,7 +83,10 @@ function createNestedClosureComponentCell(componentCell, params, hash) { return { [COMPONENT_PATH]: componentCell[COMPONENT_PATH], - [COMPONENT_HASH]: mergeInNewHash(componentCell[COMPONENT_HASH], hash), + [COMPONENT_HASH]: mergeInNewHash(componentCell[COMPONENT_HASH], + hash, + componentCell[COMPONENT_POSITIONAL_PARAMS], + params), [COMPONENT_POSITIONAL_PARAMS]: componentCell[COMPONENT_POSITIONAL_PARAMS], [COMPONENT_CELL]: true }; @@ -124,6 +128,50 @@ function getPositionalParams(container, componentPath) { } } -export function mergeInNewHash(original, updates) { - return assign({}, original, updates); +/* + * This function merges two hashes in a new one. + * Furthermore this function deals with the issue expressed in #13742. + * + * ```hbs + * {{component (component 'link-to' 'index')}} + * ``` + * + * results in the following error + * + * > You must provide one or more parameters to the link-to component. + * + * This is so because a naive merging would not take into account that the + * invocation (the external `{{component}}`) would result in the following + * attributes (before merging with the ones in the contextual component): + * + * ```js + * let attrs = { params: [] }; + * ``` + * + * Given that the contextual component has the following attributes: + * + * ```js + * let attrs = { params: ['index'] }; + * ``` + * + * Merging them would result in: + * + * ```js + * let attrs = { params: [] }; + * ``` + * + * Therefore, if there are no positional parameters and `positionalParams` is + * a string (rest positional parameters), we keep the parameters from the + * `original` hash. + * + */ +export function mergeInNewHash(original, updates, positionalParams=[], params=[]) { + let newHash = assign({}, original, updates); + + if (isRestPositionalParams(positionalParams) && isEmpty(params)) { + let propName = positionalParams; + newHash[propName] = original[propName]; + } + + return newHash; } diff --git a/packages/ember-htmlbars/lib/keywords/element-component.js b/packages/ember-htmlbars/lib/keywords/element-component.js index adc0debd217..fb51c92e468 100644 --- a/packages/ember-htmlbars/lib/keywords/element-component.js +++ b/packages/ember-htmlbars/lib/keywords/element-component.js @@ -1,7 +1,8 @@ import assign from 'ember-metal/assign'; import { - COMPONENT_PATH, COMPONENT_HASH, + COMPONENT_PATH, + COMPONENT_POSITIONAL_PARAMS, isComponentCell, mergeInNewHash, processPositionalParamsFromCell, @@ -69,8 +70,11 @@ function render(morph, env, scope, [path, ...params], hash, template, inverse, v // This needs to be done in each nesting level to avoid raising assertions processPositionalParamsFromCell(closureComponent, params, hash); + hash = mergeInNewHash(closureComponent[COMPONENT_HASH], + hash, + closureComponent[COMPONENT_POSITIONAL_PARAMS], + params); params = []; - hash = mergeInNewHash(closureComponent[COMPONENT_HASH], hash); } let templates = { default: template, inverse }; diff --git a/packages/ember-htmlbars/lib/utils/extract-positional-params.js b/packages/ember-htmlbars/lib/utils/extract-positional-params.js index d4e6d617b12..d12545818a4 100644 --- a/packages/ember-htmlbars/lib/utils/extract-positional-params.js +++ b/packages/ember-htmlbars/lib/utils/extract-positional-params.js @@ -10,8 +10,12 @@ export default function extractPositionalParams(renderNode, component, params, a } } +export function isRestPositionalParams(positionalParams) { + return typeof positionalParams === 'string'; +} + export function processPositionalParams(renderNode, positionalParams, params, attrs, raiseAssertions = true) { - let isRest = typeof positionalParams === 'string'; + let isRest = isRestPositionalParams(positionalParams); if (isRest) { processRestPositionalParameters(renderNode, positionalParams, params, attrs, raiseAssertions);