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

Handle elements in non-children props in shallow render #24

Merged
merged 1 commit into from
Nov 1, 2016

Conversation

stevoland
Copy link
Contributor

Currently React elements in props other than children aren't processed so snapshots will end up with toString()ed component types.

This is a breaking change for those with existing snapshots of elements containing elements in non-children props.

@codecov-io
Copy link

codecov-io commented Oct 31, 2016

Current coverage is 100% (diff: 100%)

Merging #24 into master will not change coverage

@@           master   #24   diff @@
===================================
  Files           3     3          
  Lines          38    46     +8   
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
+ Hits           38    46     +8   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update c029ba9...c542b80

@adriantoine
Copy link
Owner

Hi @stevoland, thanks for your contribution! Looks pretty neat, I can't review it properly right now, I'll have a look later this afternoon, sorry!

obj[key] = nodeToJson(node[key]);
});

return obj;
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer that to use Object.entries and reduce:

return Object.entries(node).reduce((obj, [key, val]) => {
    obj[key] = nodeToJson(val);
    return obj;
}, {});

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, didn't know about object.entries. will change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks

Copy link
Owner

Choose a reason for hiding this comment

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

Ah sorry, tests are failing because Object.entries is not available in Node v4. Can you use object.entries?

import entries from 'object.entries';

// ...

return entries(node).reduce(...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Owner

Choose a reason for hiding this comment

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

Nice

@adriantoine adriantoine merged commit 3acf191 into adriantoine:master Nov 1, 2016
@adriantoine
Copy link
Owner

Merged 🎉 Really nice @stevoland, thanks a lot for your contribution 👍 It will go in the next published release very soon.

@adriantoine
Copy link
Owner

The change is published in v1.2.0 🎉

@stevoland
Copy link
Contributor Author

stevoland commented Nov 1, 2016

I'm afraid you might want consider reverting this. I've just noticed that when testing react-native, a previous snapshot of:

<AnimatedComponent
            opacity={0}
/>

is now

<AnimatedComponent
            opacity={
              Object {
                "_animation": Object {
                  "__active": true,
                  "__isInteraction": true,
                  "__onEnd": [Function],
                 ...
/>

I'll try to fix this afternoon. Sorry :/

@adriantoine
Copy link
Owner

Ah crap, it might be a good thing to have React Native unit tests but I'm not sure how to do that as I'm not familiar with React Native.

@stevoland
Copy link
Contributor Author

Should have a fix in an hour. I think using lodash isPlainObject instead of simple typeof check for object will fix

@stevoland
Copy link
Contributor Author

This should fix it: #25

Sorry again!

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.

None yet

3 participants