React: Warn if stringifying an attribute takes too long

[gaearon]

With React 16 we don't have an attribute whitelist so both src={obj} and myattribute={obj} would be valid. The objects get stringified and added as attributes for smooth migration path because a lot of the existing code already depends on this behavior.

There is, however, one pitfall here. Sometimes you made do <div {...rest}> and not realize that rest includes an object whose stringifying is unusually expensive. For example a deeply nested Immutable Map. Now, this wouldn't produce an error, but it would slow down rendering for no good reason.

We could protect against this by putting performance.now() counters around the places where we stringify attributes. If stringification takes more than, say, 2 milliseconds, then something bad is going on, and we should probably warn.

[Swieckowski]

@gaearon I'd like to claim this.

[gaearon]

Sounds good!

[Swieckowski]

Could you point in the right direction for where the migration path starts for attributes? I've been looking at the PRs mentioned in https://reactjs.org/blog/2017/09/08/dom-attributes-in-react-16.html to try figure out where to start looking, but it's been a little overwhelming.

[aweary]

@Swieckowski I think you'll want to look at setValueForProperty in DOMPropertyOperations

https://github.com/facebook/react/blob/fb85cf2e9c52a4b2999144a89a6ee8256aec55c7/packages/react-dom/src/client/DOMPropertyOperations.js#L164-L166

https://github.com/facebook/react/blob/fb85cf2e9c52a4b2999144a89a6ee8256aec55c7/packages/react-dom/src/client/DOMPropertyOperations.js#L136

Those two lines are where known attribute and unknown attribute values get stringified, respectively. I'm not sure if @gaearon intends to only warn for attributes that we don't recognize, but this should be a good place to start.

Keep in mind that this warning and timing logic should only happen in the DEV bundle. This is handled by wrapping code in __DEV__ checks, which you can see examples of in the file linked above.

Hope that helps!

[Swieckowski]

I'm getting 'ReferenceError: Performance is not defined', does this have something to do with the files being in client? Date.now() works fine on the other hand.

[Swieckowski]

I'm also not sure how to reliably test something that involves a performance issue across different machines, so is this something I should write a test for?

[aweary]

I'm getting 'ReferenceError: Performance is not defined', does this have something to do with the files being in client? Date.now() works fine on the other hand.

Make sure you're using performance not Performance (lowercase "p"). It should be supported in all major browsers. We want to be sure we're accurately measuring the timing, so Date.now isn't a good choice given its low resolution compared to performance.now

I'm also not sure how to reliably test something that involves a performance issue across different machines, so is this something I should write a test for?

We should write a test and use a value that will practically always be an order of magnitude or more expensive than our threshold. For example, if we set the threshold to 2 milliseconds we should test it with something that takes ~200ms+ to stringify.

You could try overriding toString and doing some expensive work that will essentially always take too long.

```js
const attributeValue = {
toString() {
// do some expensive work here, like computing a hash
}
}

[Swieckowski]

@aweary I made sure to use the proper capitalization, but I got the reference error in tests no matter what I tried.

This is the function I made that's only used in DEV.
````
// Only used in DEV, stringifies a value and Warns if it took too long
const stringifyWithPerformanceWarning = (value) => {
const stringifyStart = performance.now();
let attributeValue = (value: any).toString();
const stringifyEnd = performance.now();

warning(stringifyEnd - stringifyStart <= 2, 'Stringifying your attribute is causing perfomance issues.')

return attributeValue;
}
````
I got reminded of project euler when thinking about expensive functions, and decided I'd just insert a find nth prime algorithm to a suitable number. I get ~200ms for the 2000th prime on my machine.

[Swieckowski]

Short version: I'm guessing it's alright to just test non-primitive data types?

Longer learning experience version:
Technically the attribute values could be any type, including primitives. I've started researching how function calls like "example".toString() or (5).toString() work in JS and it's kind of complex.

From my understanding primitives are briefly coerced into objects during the call, so there really isn't an instance method to override. I'd have to change the actual {Insert Primitve}.prototype to override them, and it rubs be the wrong way to pollute an actual datatype prototype, especially with a time wasting function.

I'm just going to go forward with the assumption that it's not that big a deal that I'm not testing primitives. Sorry if I'm being pedantic!

[aweary]

I made sure to use the proper capitalization, but I got the reference error in tests no matter what I tried.

Huh, I assumed jsdom had support for performance already but it was just added within the last month (https://github.com/jsdom/jsdom/pull/2094) so the version we depend on doesn't have it.

We could potentially polyfill it in the test setup. That would let us also implement our own version of it that always returned times in increments greater than our threshold, so we could avoid having to actually wait 200ms+ which I think is a better idea.

That raises the question, do we want to fall back to Date.now when performance.now isn't available? We do so in the scheduler, but since this is just a warning and most people develop in a browser that supports performance it might not matter.

@gaearon your thoughts?

[wuweiweiwu]

@gaearon @aweary Seems that the PR has stalled... Can I pick this up? :)

[aweary]

@wuweiweiwu it stalled because we didn't provide any feedback. I just requested some changes, so let's give @Swieckowski some time to address those requests before handing it off :)

[wuweiweiwu]

Sounds good! Thanks for letting me know!

[armujahid]

@Swieckowski Can I pick this up if you don't plan to work on it? I can work on top of your fork

[pavelzubov]

@aweary @Swieckowski can i take it?

[kambleaa007]

@aweary @Swieckowski can i take it?

Hi, can you help fixing this, https://github.com/facebook/react/pull/16315

[lmerotta]

@gaearon Hi ! I know this issue is a bit old, but can I take it ?

[kambleaa007]

@lmerotta Sure, go ahead, ok from my side, make sure its not outdated, my PR had some outdated code as its old