From 821270bdd1345e51c41de33e3f117199b0d11dca Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Fri, 29 Jun 2018 17:19:44 -0700 Subject: [PATCH] [bugfix] Properly alias cellValue (#567) * [bugfix] Properly alias cellValue cellValue is a dynamically computed value that depends on the row value and a dynamic key (the `column.valuePath` property). Currently it updates the value once, and never after that. It can't respond to external changes, and changes can't be bound. This introduces a computed property that aliases based on a dynamic key. It's based on the work in ember-macro-helpers, simplified a bit since we don't need all the functionality in there. It could possibly be simplified further in the future, but should get us where we need to be for now. * fix older ember --- .travis.yml | 2 +- addon/-private/utils/computed.js | 119 ++++++++++++++++++++++ addon/components/-private/row-wrapper.js | 10 +- addon/components/ember-td/component.js | 23 +++-- addon/components/ember-td/template.hbs | 1 - addon/components/ember-tr/template.hbs | 5 +- config/ember-try.js | 6 +- package.json | 2 +- tests/integration/components/cell-test.js | 69 ++++++++++++- 9 files changed, 211 insertions(+), 26 deletions(-) create mode 100644 addon/-private/utils/computed.js diff --git a/.travis.yml b/.travis.yml index 99b373746..0f692de68 100644 --- a/.travis.yml +++ b/.travis.yml @@ -20,7 +20,7 @@ global: env: # we recommend new addons test the current and previous LTS # as well as latest stable release (bonus points to beta/canary) - - EMBER_TRY_SCENARIO=ember-1.11 + - EMBER_TRY_SCENARIO=ember-1.12 - EMBER_TRY_SCENARIO=ember-1.13 - EMBER_TRY_SCENARIO=ember-lts-2.8 - EMBER_TRY_SCENARIO=ember-lts-2.18 diff --git a/addon/-private/utils/computed.js b/addon/-private/utils/computed.js new file mode 100644 index 000000000..404457a8a --- /dev/null +++ b/addon/-private/utils/computed.js @@ -0,0 +1,119 @@ +import EmberObject, { defineProperty, computed, observer } from '@ember/object'; +import { addListener } from '@ember/object/events'; +import { alias } from '@ember/object/computed'; +import { macro } from '@ember-decorators/object/computed'; + +const PROPERTIES = new WeakMap(); + +function findOrCreatePropertyInstance(propertyClass, context, key) { + if (!PROPERTIES.has(context)) { + PROPERTIES.set(context, {}); + } + + let propertiesForContext = PROPERTIES.get(context); + + let property = propertiesForContext[key]; + if (property) { + return property; + } + + property = propertyClass.create({ + _key: key, + _context: context, + }); + + addListener( + context, + 'willDestroy', + () => { + property.destroy(); + }, + true + ); + + propertiesForContext[key] = property; + return property; +} + +const ClassBasedComputedProperty = EmberObject.extend({ + _context: null, + _key: null, + _computedFunction: null, + _dependencies: null, + + init() { + this._redefineProperty(); + }, + + // eslint-disable-next-line + _contentDidChange: observer('_content', function() { + if (!this._isUpdating) { + this._context.notifyPropertyChange(this._key); + } + }), + + _redefineProperty() { + let dependencies = this.get('_dependencies'); + let isDynamicList = this.get('_isDynamicList'); + + let computed = this._computedFunction( + ...dependencies.map((d, i) => (isDynamicList[i] ? this.get(d) : d)) + ); + + defineProperty(this, '_content', computed); + }, + + _get() { + return this.get('_content'); + }, + + _set(key, value) { + this._isUpdating = true; + this.set('_content', value); + this._isUpdating = false; + + return this._get(); + }, +}); + +function classComputedProperty(isDynamicList, computedFunction) { + return function(...dependencies) { + let extension = { + _computedFunction: computedFunction, + _isDynamicList: isDynamicList, + _dependencies: dependencies, + }; + + dependencies.forEach((dep, index) => { + extension[dep] = alias(`_context.${dep}`); + + if (isDynamicList[index] === true) { + // eslint-disable-next-line + extension[`${dep}DidChange`] = observer(`_context.${dep}`, function() { + this._redefineProperty(); + }); + } + }); + + let klass = ClassBasedComputedProperty.extend(extension); + + return computed(...dependencies, { + get(key) { + let property = findOrCreatePropertyInstance(klass, this, key); + + return property._get(); + }, + set(key, value) { + let property = findOrCreatePropertyInstance(klass, this, key); + + return property._set(key, value); + }, + }); + }; +} + +export const dynamicAlias = macro( + classComputedProperty([false, true], function(...segments) { + return alias(segments.join('.')); + }) +); diff --git a/addon/components/-private/row-wrapper.js b/addon/components/-private/row-wrapper.js index b05d3aa45..87daf141f 100644 --- a/addon/components/-private/row-wrapper.js +++ b/addon/components/-private/row-wrapper.js @@ -10,17 +10,13 @@ import { argument } from '@ember-decorators/argument'; import { computed } from '@ember-decorators/object'; import { objectAt } from '../../-private/utils/array'; +import { dynamicAlias } from '../../-private/utils/computed'; import { guidFor } from '@ember/object/internals'; class CellWrapper extends EmberObject { - @computed('rowValue', 'columnValue.valuePath') - get cellValue() { - let row = get(this, 'rowValue'); - let valuePath = get(this, 'columnValue.valuePath'); - - return get(row, valuePath); - } + @dynamicAlias('rowValue', 'columnValue.valuePath') + cellValue; @computed('rowValue', 'columnValue.valuePath') get cellMeta() { diff --git a/addon/components/ember-td/component.js b/addon/components/ember-td/component.js index aa25df341..c350d7e1c 100644 --- a/addon/components/ember-td/component.js +++ b/addon/components/ember-td/component.js @@ -2,7 +2,7 @@ import Component from '@ember/component'; import { htmlSafe } from '@ember/string'; import { action, computed } from '@ember-decorators/object'; -import { readOnly, equal } from '@ember-decorators/object/computed'; +import { alias, readOnly, equal } from '@ember-decorators/object/computed'; import { tagName, attribute, className } from '@ember-decorators/component'; import { argument } from '@ember-decorators/argument'; import { type, optional } from '@ember-decorators/argument/type'; @@ -63,17 +63,22 @@ export default class EmberTd extends Component { @type(optional(Action)) onDoubleClick; - @readOnly('api.cellValue') cellValue; - @readOnly('api.cellMeta') cellMeta; + @computed('api.api') + get unwrappedApi() { + return this.get('api.api') || this.get('api'); + } + + @alias('unwrappedApi.cellValue') cellValue; + @readOnly('unwrappedApi.cellMeta') cellMeta; - @readOnly('api.columnValue') columnValue; - @readOnly('api.columnMeta') columnMeta; + @readOnly('unwrappedApi.columnValue') columnValue; + @readOnly('unwrappedApi.columnMeta') columnMeta; - @readOnly('api.rowValue') rowValue; - @readOnly('api.rowMeta') rowMeta; + @readOnly('unwrappedApi.rowValue') rowValue; + @readOnly('unwrappedApi.rowMeta') rowMeta; - @readOnly('api.rowSelectionMode') rowSelectionMode; - @readOnly('api.checkboxSelectionMode') checkboxSelectionMode; + @readOnly('unwrappedApi.rowSelectionMode') rowSelectionMode; + @readOnly('unwrappedApi.checkboxSelectionMode') checkboxSelectionMode; @className @equal('columnMeta.index', 0) diff --git a/addon/components/ember-td/template.hbs b/addon/components/ember-td/template.hbs index 05d9f1b6a..11de8bd88 100644 --- a/addon/components/ember-td/template.hbs +++ b/addon/components/ember-td/template.hbs @@ -44,4 +44,3 @@ {{cellValue}} {{/if}} {{/if}} - diff --git a/addon/components/ember-tr/template.hbs b/addon/components/ember-tr/template.hbs index 8c5064804..f9f17b04d 100644 --- a/addon/components/ember-tr/template.hbs +++ b/addon/components/ember-tr/template.hbs @@ -16,6 +16,8 @@ )}} {{else}} {{yield (hash + api=api + cellValue=api.cellValue cellMeta=api.cellMeta @@ -25,9 +27,6 @@ rowValue=api.rowValue rowMeta=api.rowMeta - rowSelectionMode=api.rowSelectionMode - checkboxSelectionMode=api.checkboxSelectionMode - cell=(component "ember-td" api=api) )}} {{/if}} diff --git a/config/ember-try.js b/config/ember-try.js index 54081c077..c5ca4e3d5 100644 --- a/config/ember-try.js +++ b/config/ember-try.js @@ -10,15 +10,15 @@ module.exports = function() { useYarn: true, scenarios: [ { - name: 'ember-1.11', + name: 'ember-1.12', bower: { dependencies: { - ember: '~1.11.0', + ember: '~1.12.0', 'ember-cli-shims': 'ember-cli/ember-cli-shims#0.0.3', 'ember-data': '~1.13.0', }, resolutions: { - ember: '~1.11.0', + ember: '~1.12.0', 'ember-cli-shims': '0.0.3', 'ember-data': '~1.13.0', }, diff --git a/package.json b/package.json index 825fdae41..0a5d38dec 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "ember-cli-sass": "^7.0.0", "ember-cli-version-checker": "^2.1.2", "ember-compatibility-helpers": "^1.0.0", - "ember-decorators": "^2.0.0-beta.1", + "ember-decorators": "^2.2.0", "ember-getowner-polyfill": "^2.2.0", "ember-legacy-class-shim": "^1.0.0", "ember-raf-scheduler": "^0.1.0", diff --git a/tests/integration/components/cell-test.js b/tests/integration/components/cell-test.js index 9acfd9e1f..90ce9d63e 100644 --- a/tests/integration/components/cell-test.js +++ b/tests/integration/components/cell-test.js @@ -1,9 +1,15 @@ import { module, test } from 'ember-qunit'; +import hbs from 'htmlbars-inline-precompile'; -import { generateTable } from '../../helpers/generate-table'; +import { generateTable, generateColumns } from '../../helpers/generate-table'; import { componentModule } from '../../helpers/module'; +import { set, get } from '@ember/object'; + +import { fillIn } from 'ember-native-dom-helpers'; +import wait from 'ember-test-helpers/wait'; import TablePage from 'ember-table/test-support/pages/ember-table'; +import { run } from '@ember/runloop'; let table = new TablePage(); @@ -51,4 +57,65 @@ module('Integration | cell', function() { await table.getCell(0, 0).doubleClick(); }); }); + + componentModule('mutation', function() { + test('it updates cell values when changed externally', async function(assert) { + let columnCount = 2; + let rows = [ + { + A: 'A', + B: 'B', + }, + ]; + + await generateTable(this, { rows, columnCount }); + + assert.equal(table.getCell(0, 0).text, 'A', 'renders correct initial value'); + assert.equal(table.getCell(0, 1).text, 'B', 'renders correct initial value'); + + run(() => { + set(rows[0], 'A', 'Y'); + set(rows[0], 'B', 'Z'); + }); + + await wait(); + + assert.equal(table.getCell(0, 0).text, 'Y', 'renders correct updated value'); + assert.equal(table.getCell(0, 1).text, 'Z', 'renders correct updated value'); + }); + + test('Can update cell values directly', async function(assert) { + let columnCount = 1; + let rows = [ + { + A: 'A', + }, + ]; + + this.set('columns', generateColumns(columnCount)); + this.set('rows', rows); + + this.render(hbs` +
+ {{#ember-table as |t|}} + {{ember-thead api=t columns=columns}} + + {{#ember-tbody api=t rows=rows as |b|}} + {{#ember-tr api=b as |r|}} + {{#ember-td api=r as |cellValue|}} + {{input value=cellValue}} + {{/ember-td}} + {{/ember-tr}} + {{/ember-tbody}} + {{/ember-table}} +
+ `); + + await wait(); + + fillIn('input', 'Z'); + + assert.equal(get(rows[0], 'A'), 'Z', 'value updated successfully'); + }); + }); });