Skip to content

Commit

Permalink
fix #2192 - dont recycle frozen objects (#2193)
Browse files Browse the repository at this point in the history
Summary:
This is one possible fix for #2192, which simply checks if objects are frozen before attempting to modify them.

Do note that while this fixes my use case and any others that are serialized as simple JSON-compatible types, this won't fix cases where a custom scalar is [hydrated to a more complex type](#91), such as a `Date` object. For this, you may want to consider adding a check to skip the recycle if the inputs aren't simple objects or arrays:

```es6
if (
  Object.getPrototypeOf(nextData) !== Object.getPrototypeOf(prevData) ||
  (
    Object.getPrototypeOf(nextData) !== Object.prototype &&
    Object.getPrototypeOf(nextData) !== Array.prototype
  )
) {
  return nextData;
}
```
But, since this may have more consequences, and I would really like my use case fixed ASAP 😜 I've omitted that "fix" from this PR. Please let me know if you'd like me to open a separate issue for that.
Pull Request resolved: #2193

Reviewed By: josephsavona

Differential Revision: D6845724

Pulled By: jstejada

fbshipit-source-id: 2a96bb45b349334db51304bac393b7539ee00448
  • Loading branch information
mike-marcacci authored and facebook-github-bot committed Jan 25, 2019
1 parent c1422d3 commit 4173a5e
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 5 deletions.
31 changes: 30 additions & 1 deletion packages/relay-runtime/util/__tests__/recycleNodesInto-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ describe('recycleNodesInto', () => {
const recycled = recycleNodesInto(prevData, nextData);

expect(recycled).not.toBe(prevData);
expect(recycled.bar).toBe(prevData.bar);
expect(recycled.foo).toBe(prevData.foo);
});

it('recycles identical objects', () => {
Expand All @@ -105,6 +105,28 @@ describe('recycleNodesInto', () => {
expect(recycled).toBe(prevData);
});

it('does not mutate frozen equal parent objects with equal leaf objects', () => {
const prevData = {foo: {bar: 1}};
const nextData = {foo: {bar: 1}};
Object.freeze(nextData);
Object.freeze(nextData.foo);
const recycled = recycleNodesInto(prevData, nextData);

expect(recycled).toBe(prevData);
expect(recycled.foo).toBe(prevData.foo);
});

it('does not mutate frozen unequal parent objects with equal leaf objects', () => {
const prevData = {foo: {bar: 1}, baz: 2};
const nextData = {foo: {bar: 1}, baz: 200};
Object.freeze(nextData);
Object.freeze(nextData.foo);
const recycled = recycleNodesInto(prevData, nextData);

expect(recycled).not.toBe(prevData);
expect(recycled.foo).not.toBe(prevData.foo);
});

it('does not recycle arrays as objects', () => {
const prevData = [1, 2];
const nextData = {0: 1, 1: 2};
Expand Down Expand Up @@ -135,6 +157,13 @@ describe('recycleNodesInto', () => {
expect(recycleNodesInto(prevData, nextData)).toBe(prevData);
});

it('recycles arrays with equal objects without mutating frozen `nextData`', () => {
const prevData = [{foo: 1}, {bar: 2}];
const nextData = [{foo: 1}, {bar: 2}];
Object.freeze(nextData);
expect(recycleNodesInto(prevData, nextData)).toBe(prevData);
});

it('recycles arrays without mutating `prevData`', () => {
const prevItem = {foo: 1};
const prevData = [prevItem];
Expand Down
20 changes: 16 additions & 4 deletions packages/relay-runtime/util/recycleNodesInto.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,15 @@ function recycleNodesInto<T>(prevData: T, nextData: T): T {
const prevValue = prevArray[ii];
const nextValue = recycleNodesInto(prevValue, nextItem);
if (nextValue !== nextArray[ii]) {
nextArray[ii] = nextValue;
if (__DEV__) {
if (!Object.isFrozen(nextArray)) {
nextArray[ii] = nextValue;
}
} else {
nextArray[ii] = nextValue;
}
}
return wasEqual && nextArray[ii] === prevArray[ii];
return wasEqual && nextValue === prevArray[ii];
}, true) && prevArray.length === nextArray.length;
} else if (!prevArray && !nextArray) {
// Assign local variables to preserve Flow type refinement.
Expand All @@ -49,9 +55,15 @@ function recycleNodesInto<T>(prevData: T, nextData: T): T {
const prevValue = prevObject[key];
const nextValue = recycleNodesInto(prevValue, nextObject[key]);
if (nextValue !== nextObject[key]) {
nextObject[key] = nextValue;
if (__DEV__) {
if (!Object.isFrozen(nextObject)) {
nextObject[key] = nextValue;
}
} else {
nextObject[key] = nextValue;
}
}
return wasEqual && nextObject[key] === prevObject[key];
return wasEqual && nextValue === prevObject[key];
}, true) && prevKeys.length === nextKeys.length;
}
return canRecycle ? prevData : nextData;
Expand Down

0 comments on commit 4173a5e

Please sign in to comment.