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

Unperformant Stringify Warning - WIP #12263

Closed
Closed
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
38 changes: 38 additions & 0 deletions packages/react-dom/src/__tests__/DOMPropertyOperations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,5 +155,43 @@ describe('DOMPropertyOperations', () => {
expect(container.firstChild.getAttribute('value')).toBe('foo');
expect(container.firstChild.value).toBe('foo');
});

it('should warn if custom attributes take too long to stringify', () => {
const container = document.createElement('div');
const attributeValue = {foo: 'bar'};
attributeValue.toString = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, We don't need actual implementation (which will slow down the test suite). I think better do polyfill global.performance in the test setup and implement our own version of it (like @aweary mentioned in #12209 (comment)).

For an idea, we can take a look at

global.performance = {
now() {
return now;
},
};

or jest.mockImplementation might be helpful.

Hope this help :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's go ahead and go with mocking performance instead of actually taking too much time.

// finds 2000th prime to waste time
nthPrime(1225);
let originalToString = Object.prototype.toString;
return originalToString.apply(this);
};

const nthPrime = n => {
let sieve = [2];
let current = 3;
let prime;

while (sieve.length < n) {
current += 2;
if (current % 2 === 0) {
continue;
}
prime = true;
for (let j in sieve) {
if (current % sieve[j] === 0) {
prime = false;
break;
}
}
if (prime) {
sieve.push(current);
}
}
return current;
};
expect(() =>
ReactDOM.render(<div data-foo={attributeValue} />, container),
).toWarnDev('Stringifying your attribute is causing perfomance issues');
});
});
});
26 changes: 24 additions & 2 deletions packages/react-dom/src/client/DOMPropertyOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
BOOLEAN,
OVERLOADED_BOOLEAN,
} from '../shared/DOMProperty';
import warning from 'fbjs/lib/warning';

import type {PropertyInfo} from '../shared/DOMProperty';

Expand Down Expand Up @@ -132,8 +133,13 @@ export function setValueForProperty(
const attributeName = name;
if (value === null) {
node.removeAttribute(attributeName);
} else if (__DEV__) {
node.setAttribute(
attributeName,
stringifyWithPerformanceWarning(value),
);
} else {
node.setAttribute(attributeName, '' + (value: any));
node.setAttribute(attributeName, (value: any).toString());

Choose a reason for hiding this comment

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

isn't this will throw on undefined value? we only strictly check for null above it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, @Swieckowski please revert this. We were intentionally not using toString

}
}
return;
Expand All @@ -160,10 +166,12 @@ export function setValueForProperty(
let attributeValue;
if (type === BOOLEAN || (type === OVERLOADED_BOOLEAN && value === true)) {
attributeValue = '';
} else if (__DEV__) {
attributeValue = stringifyWithPerformanceWarning(value);
} else {
// `setAttribute` with objects becomes only `[object]` in IE8/9,
// ('' + value) makes it output the correct toString()-value.
attributeValue = '' + (value: any);
attributeValue = (value: any).toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed? The comment above no longer reflects the actual code. Aren't we concerned about IE8/9 anymore? If not, then the comment should also be updated, I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@Swieckowski please revert his back as well.

}
if (attributeNamespace) {
node.setAttributeNS(attributeNamespace, attributeName, attributeValue);
Expand All @@ -172,3 +180,17 @@ export function setValueForProperty(
}
}
}

// Only used in DEV, stringifies a value and Warns if it took too long
const stringifyWithPerformanceWarning = value => {
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency let's move this to the top of the file, and then only define it in DEV

let stringifyWithPerformanceWarning;

if (__DEV__) {
  stringifyWithPerformanceWarning = function(value) {
   // ...
  }
}
``

const stringifyStart = Date.now();
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to use performance.now if it exists, and fallback to Date.now if it doesn't. Lets do the same thing we do here: https://github.com/facebook/react/blob/master/packages/react-native-renderer/src/ReactNativeFrameScheduling.js#L12-L17

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot for the review, I'll get on it sometime soon!

let attributeValue = (value: any).toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Use const over let.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use '' + value instead of toString here as well.

const stringifyEnd = Date.now();

warning(
stringifyEnd - stringifyStart <= 2,
'Stringifying your attribute is causing perfomance issues',
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: pefomance => performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

The messaging here isn't clear enough. Let's also pass the attribute name to stringifyWithPerformanceWarning, improve the wording, and also include a component stack. You can get a component stack by importing and using getCurrentFiberStackAddendum from ReactDebugCurrentFiber.

Lets make it look like:

warning(
  stringifyEnd - stringifyStart <= 2,
  'The attribute `%s` took more than 2 milliseconds to stringify. This usually means you provided a large object ' +
  'as the value for a DOM attribute, which can lead to performance issues.%s',
  attributeName,
  getCurrentFiberStackAddendum(),
)

);

return attributeValue;
};