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

Fix wrong name printed in custom elements (when passed as props) #46

Merged
merged 5 commits into from
Nov 1, 2016

Conversation

guigrpa
Copy link
Contributor

@guigrpa guigrpa commented Nov 1, 2016

When printing a custom element passed as a prop to another one with the ReactElement plugin, the name of the former is not printed correctly:

      const Cat = () => React.createElement('div');
      assertPrintedJSX(
        React.createElement('Mouse', {
          prop: React.createElement(Cat, {foo: 'bar'})
        }),
        '<Mouse\n  prop={\n    <Cat\n      foo="bar" />\n  } />'
      );

The above test fails, since the following is generated:

<Mouse\n  prop={\n    <() => React.createElement('div')\n      foo=\"bar\" />\n  } />

This issue is related to jestjs/jest#2037.

@cpojer
Copy link
Contributor

cpojer commented Nov 1, 2016

It seems like the test is actually failing on travis. Why is the element thought to be "unknown"?

@guigrpa
Copy link
Contributor Author

guigrpa commented Nov 1, 2016

The test passes on Node v6, but fails on Node v4/v5. I investigated a bit further and found that it's due to Node 6's inferred function names. Compare:

// Node v6
> foo = function() {}
[Function: foo]
> foo.name
'foo'
>

// Node v4
> foo = function() {}
[Function]
> foo.name
''
> function foo() {}
undefined
> foo.name
'foo' 

I guess that the proposed change is in any case an improvement: it fixes the problem in v6, and it provides better behaviour in v4/v5: Unknown is in any case better than the previous function dump. Maybe this test should just be skipped in previous versions of Node? (also note that v6 has recently become the current LTS branch).

@cpojer
Copy link
Contributor

cpojer commented Nov 1, 2016

Ah I see, Node 6 isn't currently part of travis. Would you mind updating that? (remove node 5 and add node 6). I recommend changing the functional component to function Cat() { return …; } so it'll work both on node 4 and node 6.

@@ -515,7 +515,7 @@ describe('prettyFormat()', () => {
});

it('supports a single element with custom React elements with props', () => {
const Cat = () => React.createElement('div');
function Cat() { React.createElement('div') };
Copy link
Contributor

Choose a reason for hiding this comment

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

you'll need to write this as

function Cat() {
  return React.createElement('div');
};

@guigrpa
Copy link
Contributor Author

guigrpa commented Nov 1, 2016

I was just writing that return, you're too fast! :)

@khankuan
Copy link
Contributor

Hmm, @thejameskyle seems like the following would result in Unknown in jest --coverage option but works fine in regular test.

const Parent = () => {
  return <MyComponent />
}

const MyComponent = () => {
  ...
}

export default Parent;

The problem seems to be multiple components definition in a single file. It feels more like an Istanbul / Jest issue. Any ideas?

@guigrpa
Copy link
Contributor Author

guigrpa commented Nov 18, 2016

I managed to reproduce it even with a single component in a file:

// ex.js
import React from 'react';
const A = () => React.createElement('span');
export default A;
// __tests__/ex.test.js
import A from '../ex';
describe('A', () => {
  it('renders correctly', () => {
    console.log(`A's name: '${A.name}'`);
  });
});

Running with --coverage gets you Unknown, without the flag A. Seems an Istanbul issue, and is indeed reported here: gotwarlost/istanbul#699.

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