Jsx ppx optimize object allocation #6376
Conversation
| @@ -26,12 +26,12 @@ module Jsx = JsxC | |||
|
|
|||
| %%private( | |||
| @val | |||
| external propsWithKey: (@as(json`{}`) _, 'props, {"key": string}) => 'props = "Object.assign" | |||
| external propsWithKey: ({"key": string}, 'props) => 'props = "Object.assign" | |||
There was a problem hiding this comment.
This was intentional to avoid mutating the object.
There was a problem hiding this comment.
Am I remembering correctly? @cknitt
There was a problem hiding this comment.
Oh, I missed that switching arguments position between key and props. It makes sense.
There was a problem hiding this comment.
It doesn't mutate the props object. It'll create the new object with key field and then mutate it.
There was a problem hiding this comment.
Before:
- Create
empty obj - Create
obj with key - Mixing
propsandobj with keyto theempty object
After:
- Create
obj with key - Mixing
propsto theobj with key
There was a problem hiding this comment.
I don't understand what you want to say 😅
There was a problem hiding this comment.
Sorry for the lack of explanation.
I meant I think that the existing implementation is more appropriate.
What do you think? @cknitt
There was a problem hiding this comment.
This should work like the previous implementation (no mutation), but with one less object allocation. Looks good to me.
There was a problem hiding this comment.
(Provided that the caller passes a different {"key": key} object every time, but that seems to be the case and this is an internal function anyway.)
There was a problem hiding this comment.
Okay. It seems unlikely that Object.assign({"key": key}, props) would change the value of key at runtime, since props would not be able to contain key when the JSX ppx transforms the component application.
LGTM.
|
Ready to merge? |
|
Yes |
|
Can you update changelog? |
|
Yep |
DZakh
force-pushed
the
react-ppx-optimize-obj-allocation
branch
from
2c8f479
to
d9286f4
Compare
No description provided.