Change typeof for prototype check to only process plain objects as mappers inputs #4258
Conversation
|
whoop whoop 🙌🏼 |
src/reanimated2/mappers.ts
Outdated
| @@ -115,7 +115,10 @@ export function createMapperRegistry() { | |||
| } | |||
| } else if (inputs.addListener) { | |||
| resultArray.push(inputs); | |||
| } else if (typeof inputs === 'object') { | |||
| } else if (inputs.__proto__ === Object.prototype) { | |||
There was a problem hiding this comment.
The __proto__ property seems to be deprecated, maybe could we use Object.getPrototypeOf?
There was a problem hiding this comment.
sure, that's a good suggestion
| @@ -115,7 +115,10 @@ export function createMapperRegistry() { | |||
| } | |||
| } else if (inputs.addListener) { | |||
| resultArray.push(inputs); | |||
| } else if (typeof inputs === 'object') { | |||
| } else if (Object.getPrototypeOf(inputs) === Object.prototype) { | |||
There was a problem hiding this comment.
not sure if that's valid scenario but if inputs is null or undefined it will throw an error
There was a problem hiding this comment.
I don't think it can be null, otherwise typescript would've alerted us
There was a problem hiding this comment.
if it was null, than the previous else if that I'm not chaging would already crash the app as it is accessing a addListener property of inputs
|
is there a way to track in which version of Reanimated this will be shipped? |
|
@wcandillon it's already released in 3.1.0 |
|
I can confirm that the fix works perfectly thank you ❤️ |
…ppers inputs (software-mansion#4258) ## Summary This PR changes the logic on when we scan nested objects provided as mappers' inputs. Before this change we'd do a `typeof` check to test whether it is an object. This check however passes on derived objects such as HostObjects, Maps, etc. Since these types of objects can have very complex structures we want to avoid scanning them recursively and only do that for plain object. We achieve this by testing its prototype and comparing with Object.prototype. We encountered this error when passing complex objects as style properties to mappers. Specifically with react-native-skia integration on [AnimateTextOnPath](https://github.com/Shopify/react-native-skia/blob/main/example/src/Examples/Reanimated/AnimateTextOnPath.tsx) example where Path object is used as mappers' input. We only found it problematic on Web however, because on Native Path object is implemented as HostObject which in turn does not support Object.values (we always get empty array for JSI's HostObjects on native). On web, however, the browser would get stuck on recursively processing this complex object which was taking very long on Firefox and was crashing the app on Chrome. ## Test plan Run test examples in the app Tested this on web with react-native-skia reanimated AnimateTextOnPath example where a HostObject is used inside a mapper and it was taking ages to process that object recursively.
|
Gang, is it possible that this fix has been only shipped for Reanimated 3? |
|
@wcandillon Yes, this PR fixes only v3. Also it cannot be simply backported to v2 because in Reanimated v2 mappers were implemented in C++ but in v3 they have been moved to JS code. |
Summary
This PR changes the logic on when we scan nested objects provided as mappers' inputs. Before this change we'd do a
typeofcheck to test whether it is an object. This check however passes on derived objects such as HostObjects, Maps, etc. Since these types of objects can have very complex structures we want to avoid scanning them recursively and only do that for plain object. We achieve this by testing its prototype and comparing with Object.prototype.We encountered this error when passing complex objects as style properties to mappers. Specifically with react-native-skia integration on AnimateTextOnPath example where Path object is used as mappers' input. We only found it problematic on Web however, because on Native Path object is implemented as HostObject which in turn does not support Object.values (we always get empty array for JSI's HostObjects on native). On web, however, the browser would get stuck on recursively processing this complex object which was taking very long on Firefox and was crashing the app on Chrome.
Test plan
Run test examples in the app
Tested this on web with react-native-skia reanimated AnimateTextOnPath example where a HostObject is used inside a mapper and it was taking ages to process that object recursively.