Skip to content

Conversation

@sophiebits
Copy link
Collaborator

Test Plan: jest. Also used ballmer-peak in IE8 to verify that it still works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're suggesting keeping checkAndWarnForMutatedProps as I have it here but getting rid of all the defineProperty stuff?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, done.

@jimfb
Copy link
Contributor

jimfb commented Dec 16, 2014

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
Copy link
Collaborator Author

Ready as far as I know. Just rebased.

Test Plan: jest. Also used ballmer-peak in IE8 to verify that it still works.
@sophiebits
Copy link
Collaborator Author

@sebmarkbage review?

@sebmarkbage
Copy link
Collaborator

Ok.@zpao, heads up that we'll need to ignore this warning internally until we've cleaned up remaining callsites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants