Skip to content

Fix for passing fragment arrays to fragments #364

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix for passing fragment arrays to fragments #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ffbbc97
Fix issue where you cannot pass fragmentArrays to fragments
patocallaghan May 29, 2020
aaa85c8
Adding a modification test to show losing the reference to the fragme…
patocallaghan May 29, 2020
003c039
use instanceof check
patocallaghan May 29, 2020
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
2 changes: 1 addition & 1 deletion addon/array/fragment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ function normalizeFragmentArray(array, content, objs, canonical) {

return objs.map((data, index) => {
let type = get(array, 'type');
assert(`You can only add '${type}' fragments or object literals to this property`, typeOf(data) === 'object' || isInstanceOfType(store.modelFor(type), data));
assert(`You can only add '${type}' fragments or object literals to the '${key}' property`, typeOf(data) === 'object' || isInstanceOfType(store.modelFor(type), data));

if (isFragment(data)) {
fragment = data;
Expand Down
2 changes: 1 addition & 1 deletion addon/array/stateful.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ const StatefulArray = ArrayProxy.extend(Copyable, {

this._pendingData = data;

let processedData = this._normalizeData(makeArray(data));
let processedData = this._normalizeData(data instanceof StatefulArray ? copy(data) : makeArray(data));
Copy link
Contributor Author

@patocallaghan patocallaghan May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igorT added an instanceof check instead of using a specific property

let content = get(this, 'content');

// This data is canonical, so create rollback point
Expand Down
35 changes: 35 additions & 0 deletions tests/unit/fragment_array_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,41 @@ module('unit - `MF.fragmentArray`', function(hooks) {
});
});

test('fragment arrays can be added to fragments', function(assert) {
let order = store.createFragment('order', {
amount: '799.98',
products: [
{
name: 'Tears of Lys',
sku: 'poison-bd-32',
price: '499.99'
},
{
name: 'The Strangler',
sku: 'poison-md-24',
price: '299.99'
}
]
});
let user = store.createRecord('user', {
info: {
name: 'Tyrion Lannister',
notes: ['smart', 'short']
},
orders: [
{
amount: '799.98',
products: order.products
}
]
});
assert.equal(user.orders.firstObject.products.firstObject.name, 'Tears of Lys', 'No problems');

order.products.firstObject.set('name', 'Wolfsbane');
assert.equal(order.products.firstObject.name, 'Wolfsbane', 'Modifies the original fragment array');
assert.equal(user.orders.firstObject.products.firstObject.name, 'Tears of Lys', 'It does not modify the passed fragment array');
Copy link
Contributor Author

@patocallaghan patocallaghan May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richgt @igorT so it appears this approach this means we lose a reference to the fragment array when we pass it in and copy it. See this test where I modify the original array to have the value 'Wolfsbane' but the value of the product within the user model hasn't changed Tears of Lys. Only the original is changed.

Digging into this a bit more, the difference between this and the old master is that on these lines when we had a fragment array, we didn't call setupData instead we just assigned it directly to the property. This meant we didn't call normalizeFragmentArray.

https://github.com/lytics/ember-data-model-fragments/blob/f1c7060fb7560aaea26760e0270234823b9e20db/addon/attributes.js#L291-L304

With the recent refactor we are now always calling setupData which means we are now normalizing the fragment array, even when it's already set up

https://github.com/lytics/ember-data-model-fragments/blob/36b38129bd9e929089a2852d1c17cd404ad91984/addon/record-data.js#L235-L237

To tell you the truth I'm not entirely sure what is the right behavior here but possibly in the spirit of the original code it should just do a simple assignment instead of re-setting up the data? I guess the original code suffered from the problem where the wrong owner is assigned to the fragmentArray?

if (serverFragment instanceof StatefulArray) {
  serverFragment[name] = data.attributes[name];
} else {
  ...
}

Copy link
Contributor Author

@patocallaghan patocallaghan Jun 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igorT @richgt I'm just wondering what's the best way to proceed here.

Solution 1

This PR in its current form.

  1. Does the copy but breaks backward compatibility
  2. Loses the reference to the fragmentArray you passed in
  3. The owner is set up correctly through setupData

Solution 2

  1. Don't re-call serverFragment.setupData(data.attributes[name]) and instead just re-assign like we did pre-5.0.
  2. Keeps the original reference to the fragment array
  3. Unfortunately the owner is now incorrect. The fragment array is still set to the initial owner. I’ve actually verified that this is existing behaviour pre-5.0 which to me seems like a bug.

Thinking about it my preference would be Solution 2, not requiring the copy, and keeping the reference. Could we augment solution 2 with also resetting the owner to be the correct one? Should we even bother considering this “bug” already existed? Not having lots of context on the refactor what's the downside to not calling serverFragment.setupData(data.attributes[name]); again.

});

test('fragments can be removed from the fragment array', function(assert) {
run(() => {
store.push({
Expand Down