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.

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

Jump to bottom

fix #2192 - dont recycle frozen objects #2193

Closed
wants to merge 1 commit into from
+36 −5
Closed

fix #2192 - dont recycle frozen objects #2193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 30 additions & 1 deletion packages/react-relay/classic/legacy/store/__tests__/recycleNodesInto-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,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 @@ -112,6 +112,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 @@ -142,6 +164,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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is definitely wrong, correct? If nextData is frozen and we don't expect to mutate it, then we would expect .toBe(nextData)

Copy link
Contributor Author

@mike-marcacci mike-marcacci Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I'm pretty sure this test is correct. It's testing that even if objects are frozen, we still recycle prevData object if it is deeply equal to nextData. This is important for preventing identical renders from triggering react diffing downstream.

The behavior of frozen objects differs from mutable ones only in that we can't preserve object equity in unchanged branches of a state tree that does have changes.

Given this prevData:

       A1
  B1        B2
C1  C2    C3  C4

we can recycle the entire object if nextData looks like this:

       A1
  B1        B2
C1  C2    C3  C4

...whether or not the anything is frozen.

However, if our nextData looks like this:

       A2
  B1        B3
C1  C2    C3  C5

...we can recycle B1 from prevData, but this requires mutating A2, which crashes the whole app if A2 is frozen. If we were really trying to optimize the frozen object case, it would be possible to create a new A2 to hold the prevData B1 and nextData B2, but since this only effects the dev environment, that's optimization that is not worth doing.

});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add another test for recycling in objects as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went to add the requested test, but it looks like I already had one that does this:
does not mutate frozen equal parent objects with equal leaf objects

So this should be good to go.


it('recycles arrays without mutating `prevData`', () => {
const prevItem = {foo: 1};
const prevData = [prevItem];
Expand Down
10 changes: 6 additions & 4 deletions packages/relay-runtime/util/recycleNodesInto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,29 +30,31 @@ function recycleNodesInto<T>(prevData: T, nextData: T): T {
const prevArray = Array.isArray(prevData) ? prevData : null;
const nextArray = Array.isArray(nextData) ? nextData : null;
if (prevArray && nextArray) {
const isFrozen = __DEV__ && Object.isFrozen(nextArray);
canRecycle =
nextArray.reduce((wasEqual, nextItem, ii) => {
const prevValue = prevArray[ii];
const nextValue = recycleNodesInto(prevValue, nextItem);
if (nextValue !== nextArray[ii]) {
if (nextValue !== nextArray[ii] && !isFrozen) {
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.
const prevObject = prevData;
const nextObject = nextData;
const prevKeys = Object.keys(prevObject);
const nextKeys = Object.keys(nextObject);
const isFrozen = __DEV__ && Object.isFrozen(nextObject);
canRecycle =
nextKeys.reduce((wasEqual, key) => {
const prevValue = prevObject[key];
const nextValue = recycleNodesInto(prevValue, nextObject[key]);
if (nextValue !== nextObject[key]) {
if (nextValue !== nextObject[key] && !isFrozen) {
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