Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding ability to use proxy objects in order to avoid object mutations to original content object #159

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions addon/components/draggable-object.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { alias } from '@ember/object/computed';
import { computed } from '@ember/object';
import { scheduleOnce, next } from '@ember/runloop';
import { set } from '@ember/object';
import { wrapper } from 'ember-drag-drop/utils/proxy-unproxy-objects';

export default Component.extend({
dragCoordinator: service(),
Expand All @@ -22,6 +23,11 @@ export default Component.extend({
return isDraggable || null;
}),

proxyContent: computed('content', function() {
return wrapper(this.get('content'));
}),


init() {
this._super(...arguments);
if (this.get('dragHandle')) {
Expand Down Expand Up @@ -68,7 +74,7 @@ export default Component.extend({

let dataTransfer = event.dataTransfer;

let obj = this.get('content');
let obj = this.get('proxyContent');
let id = null;
let coordinator = this.get('coordinator');
if (coordinator) {
Expand Down Expand Up @@ -106,7 +112,7 @@ export default Component.extend({
return;
}

let obj = this.get('content');
let obj = this.get('proxyContent');

if (obj && typeof obj === 'object') {
set(obj, 'isDraggingObject', false);
Expand Down Expand Up @@ -150,7 +156,7 @@ export default Component.extend({

actions: {
selectForDrag() {
let obj = this.get('content');
let obj = this.get('proxyContent');
let hashId = this.get('coordinator').setObject(obj, { source: this });
this.set('coordinator.clickedId', hashId);
}
Expand Down
48 changes: 48 additions & 0 deletions addon/utils/proxy-unproxy-objects.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { isNone } from '@ember/utils';
import { guidFor } from '@ember/object/internals';

/**
* On drag action we need to provide a property `isDraggingObject` to the UI.
* This utility is used to create a wrapper object around the object passed to the proxy function.
* We use this wrapper to prevent the `draggable-object` from mutating the original object by appending
* `isDraggingObject` to the content.
*
* This unexpected mutation causes problems when the targeted content is not prepared to handle
* the additional property, and potentially leaks local state onto an object that likely holds state
* for the route or application more generally.
*/

/**
* @access public
* Returns the passed object wrapped within new object.
* Used to proxy draggable objects.
* @param objectToProxy Object to proxy.
* @returns {Object} Proxy object.
*/
export function wrapper(objectToProxy) {
// If we do not have any content for the object to proxy,
// we do not wish to render that item.
if (!isNone(objectToProxy)) {
const guidKey = guidFor(objectToProxy);
return {
[guidKey]: objectToProxy,
unwrappingKey: guidKey,
id: objectToProxy.id
};
}
return null;
}

/**
* @access public
* Returns the content of the passed object.
* Used to un-proxy draggable objects.
* @param objectToUnproxy Object to un-proxy.
* @returns {Object} Content of the proxy object.
*/
export function unwrapper(objectToUnproxy) {
if (!isNone(objectToUnproxy)) {
return objectToUnproxy[objectToUnproxy.unwrappingKey];
}
return null;
}
5 changes: 3 additions & 2 deletions app/models/coordinator.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import EmberObject from '@ember/object';
import Evented from '@ember/object/evented';
import { computed } from '@ember/object';
import ObjHash from './obj-hash';
import { unwrapper } from 'ember-drag-drop/utils/proxy-unproxy-objects';

export default EmberObject.extend(Evented, {
objectMap: computed(function() {
Expand All @@ -20,9 +21,9 @@ export default EmberObject.extend(Evented, {
payload.ops.target.sendAction('action',payload.obj);
}

this.trigger("objectMoved", {obj: payload.obj, source: payload.ops.source, target: ops.target});
this.trigger("objectMoved", {obj: unwrapper(payload.obj), source: payload.ops.source, target: ops.target});

return payload.obj;
return unwrapper(payload.obj);
},

setObject: function(obj,ops) {
Expand Down
3 changes: 2 additions & 1 deletion tests/unit/components/draggable-object-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { A } from '@ember/array';
import { test, moduleForComponent } from 'ember-qunit';
import Coordinator from '../../../models/coordinator';
import MockEvent from '../../helpers/mock-event';
import { wrapper } from 'ember-drag-drop/utils/proxy-unproxy-objects';

const Thing = EmberObject.extend({});

Expand Down Expand Up @@ -62,7 +63,7 @@ test("drop callbacks", function(assert) {
let component = this.subject({ coordinator });

let hashId = run(function() {
return coordinator.setObject(thing, { source: component });
return coordinator.setObject(wrapper(thing), { source: component });
});

coordinator.getObject(hashId);
Expand Down
46 changes: 46 additions & 0 deletions tests/unit/utils/proxy-unproxy-objects-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { module, test } from 'qunit';
import { setupTest } from 'ember-qunit';
import {
wrapper,
unwrapper,
} from 'ember-drag-drop/utils/proxy-unproxy-objects';
import { guidFor } from '@ember/object/internals';

module(
'Unit | Util | ember-drag-drop/utils/proxy-unproxy-objects',
function(hooks) {
setupTest(hooks);

hooks.beforeEach(function() {
this.testObject = {
value: true,
id: 123,
};
this.testObjectGuid = guidFor(this.testObject);
});

test('wrapper returns a new object containing content and ID feilds', function testProxyObjAction(assert) {
assert.expect(2);
assert.equal(
wrapper(this.testObject)[this.testObjectGuid],
this.testObject,
'Wrapped object contains corresponding guid field containing the original object content'
);

assert.equal(
wrapper(this.testObject).id,
this.testObject.id,
'Object contains ID field'
);
});

test('unwrapper returns back the original object', function testUnproxyObjAction(assert) {
assert.expect(1);
assert.deepEqual(
unwrapper(wrapper(this.testObject)),
this.testObject,
'Returned object is same as the original test object'
);
});
}
);