Ensure state is merged into meta props as plain object (#1877, #1955) #2336

Merged
merged 1 commit into from Dec 27, 2016

Conversation

3 participants
@esbenp
Contributor

esbenp commented Dec 26, 2016

cb3abb5 added functionality that merged the state into the meta object for fields. However, when using Immutable JS structures the state will be an Immutable.Iterable. When developing in React Native the Object.assign call here cb3abb5#diff-7eb826f425b082db29894569d83899f8R68 causes an exception as described in #1877 and #1955.

One of the sources for assign has an enumerable key on the prototype chain. This is an edge case that we do not support. This error is a performance optimization and not spec compliant.

I am assuming this is because the Object.assign polyfill is different in the React Native environment (any confirmation on this?). I tested the webpack polyfill in the Immutable example and it pretty much just transfers properties from one object to another without any checks. I am assuming this is why this error has not had any more attention since it does not happen in browser environments.

EDIT: Found the polyfill https://github.com/facebook/react-native/blob/master/packager/react-packager/src/Resolver/polyfills/polyfills.js#L56

As for the fix I added a toJS method to the structure types. This also means I had to expand the first argument in createFieldProps and createFieldsProps to be an object instead of just getIn. I am not familiar with the code base so any pointers and directions for style is much appreciated. It should be noted that I have not tested this solution in React Native environment (only a dirty hack that accomplishes the same). And I have not tested all the examples either but tests are passing.

Ensure immutable state is merged into field meta props as plain object (
#1877, #1955)


cb3abb5 added functionality that merged the state
into the meta object for fields. However, when using Immutable JS structures this
will be an Immutable.Iterable. This causes an exception in environments where
the Object.assign polyfill does not allow this (for instance in React Native)
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 26, 2016

Current coverage is 100% (diff: 100%)

Merging #2336 into master will not change coverage

@@           master   #2336   diff @@
=====================================
  Files          61      61          
  Lines        1267    1269     +2   
  Methods         0       0          
  Messages        0       0          
  Branches        0       0          
=====================================
+ Hits         1267    1269     +2   
  Misses          0       0          
  Partials        0       0          

Powered by Codecov. Last update c3aa263...4eb1d47

codecov-io commented Dec 26, 2016

Current coverage is 100% (diff: 100%)

Merging #2336 into master will not change coverage

@@           master   #2336   diff @@
=====================================
  Files          61      61          
  Lines        1267    1269     +2   
  Methods         0       0          
  Messages        0       0          
  Branches        0       0          
=====================================
+ Hits         1267    1269     +2   
  Misses          0       0          
  Partials        0       0          

Powered by Codecov. Last update c3aa263...4eb1d47

@erikras

This comment has been minimized.

Show comment
Hide comment
@erikras

erikras Dec 27, 2016

Owner

Looks good.

Owner

erikras commented Dec 27, 2016

Looks good.

@erikras erikras merged commit bf698c8 into erikras:master Dec 27, 2016

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0.00%) compared to c3aa263
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot Jun 1, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

lock bot commented Jun 1, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Repository owner locked as resolved and limited conversation to collaborators Jun 1, 2018

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