Skip to content

Commit

Permalink
[refactor] Remove component references from trees (#534)
Browse files Browse the repository at this point in the history 
This refactor removes references to the `component` from trees in favor
of using observers. We still have occasional memory leaks, and this
eliminates one cycle of chains/observation. In the future when we drop
1.11 support we can use `didReceiveAttrs` to assign these values instead
of observers.
  • Loading branch information
@pzuraq
pzuraq authored May 21, 2018
1 parent 44bbc49 commit f5fd28b
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 37 deletions.
18 changes: 3 additions & 15 deletions addon/-private/collapse-tree.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class TableRowMeta extends EmberObject {
let rowMetaCache = get(tree, 'rowMetaCache');

if (selectMode === SELECT_MODE.SINGLE) {
tree.component.sendAction('onSelect', [rowValue]);
tree.sendAction('onSelect', [rowValue]);
return;
}

Expand Down Expand Up @@ -181,7 +181,7 @@ class TableRowMeta extends EmberObject {

selectedRows = emberA(Array.from(selectedRows));

tree.component.sendAction('onSelect', selectedRows);
tree.sendAction('onSelect', selectedRows);

tree._lastSelectedIndex = rowIndex;
}
Expand Down Expand Up @@ -582,19 +582,7 @@ export default class CollapseTree extends EmberObject.extend(EmberArray) {
// Whenever the root node's length changes we need to propogate the change to
// users of the tree, and since the tree is meant to work like an array we should
// trigger a change on the `[]` key as well.
addObserver(this, 'root.length', () => {
notifyPropertyChange(this, '[]');
});
}

@readOnly('component.enableCollapse') enableCollapse;
@readOnly('component.enableTree') enableTree;
@readOnly('component.selectedRows') selectedRows;

@computed('component.{selectMode,onSelect}')
get selectMode() {
let onSelect = this.get('component.onSelect');
return onSelect ? this.get('component.selectMode') : 'none';
addObserver(this, 'root.length', () => notifyPropertyChange(this, '[]'));
}

destroy() {
Expand Down
15 changes: 6 additions & 9 deletions addon/-private/column-tree.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,6 @@ export default class ColumnTree extends EmberObject {

this.token = new Token();

this.sortColumnsByFixed();
addObserver(this, 'columns.@each.isFixed', this.sortColumnsByFixed);
addObserver(this, 'widthConstraint', this.ensureWidthConstraint);
}
Expand All @@ -431,17 +430,11 @@ export default class ColumnTree extends EmberObject {
super.destroy();
this.token.cancel();
get(this, 'root').destroy();
set(this, 'component', null);

removeObserver(this, 'columns.@each.isFixed', this.sortColumnsByFixed);
removeObserver(this, 'widthConstraint', this.ensureWidthConstraint);
}

@readOnly('component.columns') columns;
@readOnly('component.fillMode') fillMode;
@readOnly('component.resizeMode') resizeMode;
@readOnly('component.widthConstraint') widthConstraint;

@computed('columns')
get root() {
let columns = get(this, 'columns');
Expand Down Expand Up @@ -543,6 +536,10 @@ export default class ColumnTree extends EmberObject {
};

ensureWidthConstraint = () => {
if (!this.container) {
return;
}

let containerWidth = getInnerClientRect(this.container).width * this.scale;
let treeWidth = get(this, 'root.width');
let columns = get(this, 'root.subcolumnNodes');
Expand Down Expand Up @@ -732,7 +729,7 @@ export default class ColumnTree extends EmberObject {

this.container.classList.remove('et-unselectable');

this.component.sendAction('onReorder', get(node, 'column'), get(closestColumn, 'column'));
this.sendAction('onReorder', get(node, 'column'), get(closestColumn, 'column'));
}

startResize(node, clientX) {
Expand Down Expand Up @@ -806,7 +803,7 @@ export default class ColumnTree extends EmberObject {

this.container.classList.remove('et-unselectable');

this.component.sendAction('onResize', get(node, 'column'));
this.sendAction('onResize', get(node, 'column'));
}

updateScroll(node, stopAtLeft, stopAtRight, callback) {
Expand Down
22 changes: 21 additions & 1 deletion addon/components/ember-tbody/component.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,17 +96,37 @@ export default class EmberTBody extends Component {
*/
rowMetaCache = new Map();

collapseTree = CollapseTree.create({ component: this, rowMetaCache: this.rowMetaCache });
collapseTree = CollapseTree.create({
sendAction: this.sendAction.bind(this),
rowMetaCache: this.rowMetaCache,
});

constructor() {
super(...arguments);

this._updateCollapseTree();

this.addObserver('enableCollapse', this._updateCollapseTree);
this.addObserver('enableTree', this._updateCollapseTree);
this.addObserver('selectedRows', this._updateCollapseTree);
this.addObserver('selectMode', this._updateCollapseTree);
this.addObserver('onSelect', this._updateCollapseTree);

assert(
'You must create an {{ember-thead}} with columns before creating an {{ember-tbody}}',
!!this.get('api.columnTree')
);
}

_updateCollapseTree() {
let onSelect = this.get('onSelect');

this.collapseTree.set('enableCollapse', this.get('enableCollapse'));
this.collapseTree.set('enableTree', this.get('enableTree'));
this.collapseTree.set('selectedRows', this.get('selectedRows'));
this.collapseTree.set('selectMode', onSelect ? this.get('selectMode') : 'none');
}

willDestroy() {
this._cleanCaches();

Expand Down
19 changes: 18 additions & 1 deletion addon/components/ember-thead/component.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,31 @@ export default class EmberTHead extends Component {
*/
columnMetaCache = new Map();

columnTree = ColumnTree.create({ component: this, columnMetaCache: this.columnMetaCache });
columnTree = ColumnTree.create({
sendAction: this.sendAction.bind(this),
columnMetaCache: this.columnMetaCache,
});

constructor() {
super(...arguments);

this._updateColumnTree();

this.addObserver('columns', this._updateColumnTree);
this.addObserver('fillMode', this._updateColumnTree);
this.addObserver('resizeMode', this._updateColumnTree);
this.addObserver('widthConstraint', this._updateColumnTree);

this.get('api').registerColumnTree(this.columnTree);
}

_updateColumnTree() {
this.columnTree.set('columns', this.get('columns'));
this.columnTree.set('fillMode', this.get('fillMode'));
this.columnTree.set('resizeMode', this.get('resizeMode'));
this.columnTree.set('widthConstraint', this.get('widthConstraint'));
}

didInsertElement() {
super.didInsertElement(...arguments);

Expand Down
24 changes: 13 additions & 11 deletions tests/unit/-private/collapse-tree-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ module('Unit | Private | CollapseTree', function(hooks) {
test('basic tree works', function(assert) {
tree = CollapseTree.create({
rows: generateTree([0, [1, [2, 3], 4, [5, 6]]]),
component: { enableTree: true },
enableTree: true,
rowMetaCache,
});

Expand All @@ -76,7 +76,7 @@ module('Unit | Private | CollapseTree', function(hooks) {
test('can disable tree', function(assert) {
tree = CollapseTree.create({
rows: generateTree([0, [1, 2]]),
component: { enableTree: false },
enableTree: false,
rowMetaCache,
});

Expand All @@ -89,7 +89,7 @@ module('Unit | Private | CollapseTree', function(hooks) {
assert.equal(metaFor(tree.objectAt(i)).get('depth'), expectedDepth[i]);
}

tree.set('component.enableTree', true);
tree.set('enableTree', true);

expectedDepth = [0, 1, 1];

Expand All @@ -104,7 +104,7 @@ module('Unit | Private | CollapseTree', function(hooks) {
test('works with multiroot tree', function(assert) {
tree = CollapseTree.create({
rows: generateTree([0, [1, [2, 3], 4, [5, 6]], 7, [8, 9]]),
component: { enableTree: true },
enableTree: true,
rowMetaCache,
});

Expand All @@ -121,7 +121,7 @@ module('Unit | Private | CollapseTree', function(hooks) {
test('intermediate leaf nodes work', function(assert) {
tree = CollapseTree.create({
rows: generateTree([0, [1, 2, [3, 4], 5, 6, [7, 8]]]),
component: { enableTree: true },
enableTree: true,
rowMetaCache,
});

Expand All @@ -140,7 +140,8 @@ module('Unit | Private | CollapseTree', function(hooks) {
tree = CollapseTree.create({
rows,
rowMetaCache,
component: { enableTree: true, enableCollapse: true },
enableTree: true,
enableCollapse: true,
});

set(rows[0].children[0], 'isCollapsed', true);
Expand Down Expand Up @@ -173,7 +174,8 @@ module('Unit | Private | CollapseTree', function(hooks) {
tree = CollapseTree.create({
rows,
rowMetaCache,
component: { enableTree: true, enableCollapse: true },
enableTree: true,
enableCollapse: true,
});

metaFor(tree.objectAt(1)).toggleCollapse();
Expand Down Expand Up @@ -207,7 +209,7 @@ module('Unit | Private | CollapseTree', function(hooks) {

test('can disable collapse', function(assert) {
let rows = generateTree([0, [1, [2, 3], 4, [5, 6]]]);
tree = CollapseTree.create({ rows, rowMetaCache, component: { enableTree: true } });
tree = CollapseTree.create({ rows, rowMetaCache, enableTree: true });

assert.equal(metaFor(tree.objectAt(1)).get('canCollapse'), false, 'collapse is disabled');

Expand All @@ -223,7 +225,7 @@ module('Unit | Private | CollapseTree', function(hooks) {
assert.equal(metaFor(tree.objectAt(i)).get('depth'), expectedDepth[i]);
}

tree.set('component.enableCollapse', true);
tree.set('enableCollapse', true);
metaFor(tree.objectAt(1)).toggleCollapse();

expectedValue = [0, 1, 4, 5, 6];
Expand All @@ -235,7 +237,7 @@ module('Unit | Private | CollapseTree', function(hooks) {
test('can update nodes', function(assert) {
let rows = generateTree([0, [1, [2, 3], 6, [7, 8]]]);
let subrows = generateTree([4, 5]);
tree = CollapseTree.create({ rows, rowMetaCache, component: { enableTree: true } });
tree = CollapseTree.create({ rows, rowMetaCache, enableTree: true });

rows[0].children[0].children.pushObjects(subrows);
rows[0].children[1].children.popObject();
Expand All @@ -253,7 +255,7 @@ module('Unit | Private | CollapseTree', function(hooks) {
test('can add and remove children', function(assert) {
let rows = generateTree([0, [1, [2, 3], 6, [7, 8]]]);
let subrows = generateTree([4, 5]);
tree = CollapseTree.create({ rows, rowMetaCache, component: { enableTree: true } });
tree = CollapseTree.create({ rows, rowMetaCache, enableTree: true });

set(rows[0].children[0].children[1], 'children', subrows);
set(rows[0].children[1], 'children', null);
Expand Down

0 comments on commit f5fd28b

Please sign in to comment.