Warn when mutating props on a ReactElement #2540
Conversation
sophiebits
force-pushed
the
no-mutate-props
branch
from
65e3d94
to
649ed7c
Compare
| warnForPropsMutation(propName, element); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I'm not sure if it's worth the perf and complexity hit on every property in the entire app. Especially since this pattern is often used to add properties rather than to mutate existing props.
Temporarily enabling Object.freeze (and strict mode) is a pretty good way to help debugging once you hit this issue.
We can probably remove the .props mutation check too.
There was a problem hiding this comment.
You're suggesting keeping checkAndWarnForMutatedProps as I have it here but getting rid of all the defineProperty stuff?
sophiebits
force-pushed
the
no-mutate-props
branch
from
649ed7c
to
ffa46a1
Compare
|
Ok, we are on a good track to have our codebase ready for this change. Anything that hasn't been fixed has tasks assigned (ie. remaining fixes in progress). Let's bring this PR back onto the front burner. Any CR comments remaining, or are we otherwise ready to merge? |
sophiebits
force-pushed
the
no-mutate-props
branch
from
ffa46a1
to
b4f651f
Compare
|
Ready as far as I know. Just rebased. |
Test Plan: jest. Also used ballmer-peak in IE8 to verify that it still works.
sophiebits
force-pushed
the
no-mutate-props
branch
from
b4f651f
to
a5aacb9
Compare
|
@sebmarkbage review? |
|
Ok.@zpao, heads up that we'll need to ignore this warning internally until we've cleaned up remaining callsites. |
Warn when mutating props on a ReactElement
Test Plan: jest. Also used ballmer-peak in IE8 to verify that it still works.