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 RST mapping to contains #1422

Closed
eddyerburgh opened this issue Dec 8, 2017 · 3 comments
Closed

Add RST mapping to contains #1422

eddyerburgh opened this issue Dec 8, 2017 · 3 comments

Comments

@eddyerburgh
Copy link
Contributor

eddyerburgh commented Dec 8, 2017

I'm making an enzyme-inferno-adapter, but I've hit some problems getting the last few tests to pass.

Enzyme requires className to be inside props. This is fine normally, because we can convert the inferno VNode tree to an enzyme RST tree.

But when you call contains, enzyme compares the RST node to the newly created VNode. The RST node has className inside props, the inferno VNode has className on the vNode.

Example:

RST Node:

{
type: 'div',
  props: {
    className: 'bar'
  }
}

Inferno VNode

type: 'div',
  props: {
  },
  className: 'bar'
}

That means we can't call a method like this:

const a = <div className="bar" />;
expect(mount(Component).contains(a)).to.equal(true);

Can we convert the node that contains is called with to an RST node before comparing?

I'm happy to make the PR

@ljharb
Copy link
Member

ljharb commented Dec 8, 2017

Hmm; I'm a bit confused. A PR with test cases might make it more clear?

@eddyerburgh
Copy link
Contributor Author

I'm not sure how to write a test case, hopefully this code makes it clear:

https://github.com/eddyerburgh/enzyme/commit/671b7aebb70380229c505c3def8eab9711327d83

In the inferno enzyme adapter, we need to add className to props. That means we need to convert any vNodes before we compare them, otherwise components with the same className will fail, because enzyme compares props in internalNodeCompare.

Currently using inferno, this:

const a = <div className="bar" />;
expect(mount(Component).contains(a)).to.equal(true);

Will always return false because it's comparing Component in RST form, to a, which hasn't been transformed. The RST node for Component will have className inside props, a does not have className inside props, because it hasn't been transformed.

This isn't a problem for React, because you don't need to transform the props object.

@ljharb
Copy link
Member

ljharb commented Dec 8, 2017

I think that makes sense.

It seems like the change you want would theoretically continue to pass on all our React tests; I'm indeed not sure how to test the need for it without making a custom adapter for that purpose.

Want to make a PR?

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

No branches or pull requests

2 participants