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

Add name method #335

Merged
merged 1 commit into from
May 8, 2016
Merged

Add name method #335

merged 1 commit into from
May 8, 2016

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Apr 19, 2016

I think I'm doing something weird here, for the shallow one. To be able to get the name, I have to wrap the component I want in another component. Not a problem for mount. But this is how type works as well, so I don't know if I'm doing something, or if it's by design.
The weird thing is the docs for type says

const wrapper = shallow(<Foo />);
expect(wrapper.type()).to.equal(Foo);

Which is false, at least for a component like

class Foo extends React.Component {
  render() {
    return <div />;
  }
}

const wrapper = shallow(<Foo />);

expect(wrapper.type()).to.equal(Foo); // Fails, it's 'div'

It works correctly using mount, though.

class Foo extends React.Component {
  render() {
    return <div />;
  }
}

const wrapper = mount(<Foo />);

expect(wrapper.type()).to.equal(Foo); // Succeeds

So I don't know if it's a bug, or if the docs for shallow is wrong.

But there is a test for it that checks the current behavior.
shallow is null: https://github.com/airbnb/enzyme/blob/1b9d317c57ba03414ae43109fa36bf4abf6e3445/test/ShallowWrapper-spec.js#L2213
mount is not: https://github.com/airbnb/enzyme/blob/1b9d317c57ba03414ae43109fa36bf4abf6e3445/test/ReactWrapper-spec.js#L1881

If it is correct, maybe the docs need to be updated?

@ljharb
Copy link
Member

ljharb commented Apr 19, 2016

I'll let @lelandrichardson comment on the shallow weirdness you mentioned.

Could you please add React.createClass and SFC tests for both shallow and mount?

@SimenB
Copy link
Contributor Author

SimenB commented Apr 19, 2016

Tests added for displayName using SFC and React.createClass.

Comments on the docs? I had no idea how to express it, so I just cobbled something together. Is there a better wording I could use?

@@ -2244,4 +2244,72 @@ describe('shallow', () => {
});
});

describe('.name()', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wrapper components here are what I'm talking about

@SimenB
Copy link
Contributor Author

SimenB commented Apr 19, 2016

I added the createClass and SFC tests only for displayName. Want me to add them for name as well?
Seems like it would be overkill, that's why I didn't

@ljharb
Copy link
Member

ljharb commented Apr 19, 2016

It would probably be best to include them, to prevent future regressions.

@SimenB
Copy link
Contributor Author

SimenB commented Apr 20, 2016

Aight, added.

@SimenB
Copy link
Contributor Author

SimenB commented Apr 28, 2016

@ljharb ping 😄

@ljharb
Copy link
Member

ljharb commented Apr 28, 2016

@SimenB i've left it for @lelandrichardson to take a final look at - although now it needs a fresh rebase ;-)

@SimenB
Copy link
Contributor Author

SimenB commented Apr 28, 2016

Ah, didn't notice. Will rebase!

EDIT: Rebased

@lelandrichardson
Copy link
Collaborator

@SimenB I'm good with this addition. Looks good.

Regarding the behavior you're mentioning above, this is expected. when you run shallow(<Foo />), you are rendering Foo, and then wrapping around the result... so the wrapper by default is looking at the root node (which in your example is a div).

Sorry this took me so long, but you think you can rebase this now? Just merged a big PR and would like things to be sequential. Thanks.

@SimenB
Copy link
Contributor Author

SimenB commented May 8, 2016

Rebased.

If I understand you correctly, that means the docs here are wrong, right?

@lelandrichardson
Copy link
Collaborator

doh. Merged something else in and now there are conflicts :(

@SimenB yes, those docs look very misleading! I'll go in and fix. Thanks for pointing it out.

@SimenB
Copy link
Contributor Author

SimenB commented May 8, 2016

Rebased against latest master

@lelandrichardson lelandrichardson merged commit 8138b73 into enzymejs:master May 8, 2016
@SimenB SimenB deleted the name branch May 9, 2016 05:40
@SimenB
Copy link
Contributor Author

SimenB commented May 9, 2016

Great, thanks!

Any plans on a new release soon? A lot of nice features has been added since 2.2 😄

EDIT: It has been released. Woop!

@jerrysu
Copy link

jerrysu commented Jun 30, 2016

The example here is also wrong per #335 (comment): https://github.com/airbnb/enzyme/blob/master/docs/api/ShallowWrapper/props.md#example

Which means that #384 is not a bug.

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

Successfully merging this pull request may close these issues.

4 participants