Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy object to prevent exception in react native #960

Merged
merged 5 commits into from
Jun 2, 2017

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented May 19, 2017

@HazAT HazAT self-assigned this May 19, 2017
@mitsuhiko
Copy link
Member

What if we do something else here and just check for Object.isFrozen(crumb) and continue? In theory since a frozen crumb means it already passed the react native bridge we should not need to do any changes to it.

@HazAT HazAT changed the title Copy object to prevent exception in react native [WIP] Copy object to prevent exception in react native May 19, 2017
@LewisJEllis
Copy link
Contributor

Object.isFrozen is IE9+, but we support IE8. Object.assign is ES2015, use our objectMerge helper instead.

Copy link
Contributor

@LewisJEllis LewisJEllis left a comment

Choose a reason for hiding this comment

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

Use objectMerge

@mitsuhiko
Copy link
Member

I would check for the existence of isFrozen and skip if we have that function and the object is frozen. That entire branch only matters for react native.

@LewisJEllis
Copy link
Contributor

Yea, sure, I don't have any opinion on what the right thing to be doing is here w.r.t. react-native etc, just want to make sure we keep compatibility in good shape.

@HazAT HazAT changed the title [WIP] Copy object to prevent exception in react native Copy object to prevent exception in react native May 30, 2017
@benvinegar
Copy link
Contributor

@HazAT – Can we add some kind of cursory test? Then I'd be glad to approve.

src/raven.js Outdated
@@ -1779,6 +1780,13 @@ function objectMerge(obj1, obj2) {
return obj1;
}

function objectFrozen(obj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@HazAT – would you mind adding a comment about why we do this for react native, and why it's okay if Object.isFrozen isn't called in environments where it isn't supported (maybe link to this PR?). Last thing and then I'll merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course makes sense 👍

@benvinegar benvinegar dismissed LewisJEllis’s stale review June 2, 2017 17:56

isFrozen check was applied

@benvinegar benvinegar merged commit c07579b into master Jun 2, 2017
@benvinegar benvinegar deleted the bugfix/react-native-freeze branch June 2, 2017 17:56
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.

4 participants