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

render to html output changed in v3 #1162

Closed
mpeyper opened this issue Sep 27, 2017 · 17 comments
Closed

render to html output changed in v3 #1162

mpeyper opened this issue Sep 27, 2017 · 17 comments

Comments

@mpeyper
Copy link

mpeyper commented Sep 27, 2017

Previously using v2.9.1 and moving to v3.0.0, the output from render(...).html() has changed.

Previously, the following test would pass:

it('should render html', () => {
    const Test = () => <p>test</p>
    expect(render(<Test />).html()).to.equal("<p>test</p>")
})

In v3, I get the following error:

AssertionError: expected 'test' to equal '<p>test</p>'
+ expected - actual

-test
+<p>test</p>

I have checked the migration guide and change log for both enzyme and cheerio, but could not find reference to this being intentional.

@ljharb
Copy link
Member

ljharb commented Sep 27, 2017

We upgraded from cheerio v0.22 to v1, so breaking changes are expected - https://github.com/airbnb/enzyme/blob/master/docs/guides/migration-from-2-to-3.md#cheerio-has-been-updated-thus-render-has-been-updated-as-well - in this case, it's due to https://github.com/cheeriojs/cheerio/blob/48eae25c93702a29b8cd0d09c4a2dce2f912d1f4/History.md#migrating-from-version-0x.

In other words, the render wrapper is now around what the component renders, so .html() is now the HTML of its contents.

You can, however, import cheerio from 'cheerio'; cheerio.html(render(<Test />)) and you'll get the result you expect.

(cc @jugglinmike)

@lelandrichardson
Copy link
Collaborator

@ljharb i'd like to understand this better, but this feels wrong to me still... I think the above test probably shouldn't have broken in this release :/

@ljharb
Copy link
Member

ljharb commented Sep 27, 2017

It would have been ideal, but the breaking changes in cheerio are such that it was unavoidable - they no longer have the concept of "root", and .html() has always been innerHTML, not outerHTML.

@mpeyper
Copy link
Author

mpeyper commented Sep 27, 2017

I can confirm, cheerio.html(render(<Test />)) gives the expected output.

Given that, would it be possible to return a wrapper around cheerio for the html call. Something like (based on render.js):

import cheerio from 'cheerio';
import { getAdapter } from './Utils';

export default function render(node, options = {}) {
  const adapter = getAdapter(options);
  const renderer = adapter.createRenderer({ mode: 'string', ...options });
  const html = renderer.render(node, options.context);
  const cheerioInstance = cheerio.load('')(html);

  return {
    ...cheerioInstance,
    html: () => cheerio.html(cheerioInstance)
  }
}

@ljharb
Copy link
Member

ljharb commented Sep 28, 2017

@mpeyper 1) that would make the .html() output different than all the other methods on cheerioInstance, 2) that would be incorrect because cheerioInstance isn't a plain object, and object spread makes it one, 3) html also can take an argument.

@mpeyper
Copy link
Author

mpeyper commented Sep 28, 2017

Obviously you know the cheerio api better than I do so I'll leave the actual implementation up to you guys. All I was suggesting was that if the cheerio api isn't ideal anymore, perhaps you could provide your own that uses cheerio under the hood.

@fzaninotto
Copy link

This breaking change is really a bummer, and a step backwards IMHO. I understand your concerns with cheerio, but to my mind this is an implementation detail that enzyme should solve to keep BC. I use enzyme, not cheerio.

If html() isn't updated to give the same result as in previous enzyme versions, I recommend to:

  • change the ShallowWrapper.html() documentation, because it still shows the previous behavior and is misleading
  • add a new method reproducing the old html() feature, because how do I test attributes of the enclosing tag otherwise?
it('should render a large paragraph', () => {
    const Test = () => <p style={{ fontSize: 13 }}>test</p>
    expect(render(<Test />).html()).to.equal('<p style="font-size: 13px">test</p>');
})

Also, writing tests like the following really doesn't make sense:

it('should render html', () => {
    const Test = () => <p>test</p>
    expect(render(<Test />).html()).to.equal("test")
});

@ljharb
Copy link
Member

ljharb commented Oct 17, 2017

it('should render a large paragraph', () => {
    const Test = () => <p style={{ fontSize: 13 }}>test</p>;
    const wrapper = render(<Test />);
    expect(wrapper.is('p')).to.equal(true);
    expect(wrapper.attr('style')).to.equal('font-size: 13px;');
    expect(render(<Test />).html()).to.equal('test');
});

Indeed, the docs should be updated.

In order to make a change here, something would have to change inside cheerio itself.

@winseros
Copy link

After those new changes the whole render API became useless for me. The pattern I was using with enzyme was:

expect(render(<Test/>).html()).to.equal('<p class="p-class">test</p>');

Now I have to use 'render' API for that which is an overkill in 80% of cases.

It would be nice if you could bring the outer HTML back at some form.

@doxiaodong
Copy link

I use render().html() for snapshot, And it would be nice if you could bring the outer HTML back at some form~~~

@jcrben
Copy link

jcrben commented Mar 29, 2018

In cheerio, it looks like had this discussion back in 2012 and came up with $.html() cheeriojs/cheerio#16 (comment) - not sure how much that helps in this case since I don't want to be importing $...

there'a slso prop(outHTML) - that may look confusing given that it mixes browser and react props... haven't tested it yet but it's theoretically in 1.0 cheeriojs/cheerio#945

@franciscop-invast
Copy link

franciscop-invast commented May 14, 2018

This has the unintended consequence that it makes .html() wildly inconsistent between mount(), render() and shallow(). This test, despite of the looks of it, is PASSING:

const base = <div><b>important</b></div>;

it('sanity test', () => {
  expect(  mount(base).html()).toEqual('<div><b>important</b></div>');
  expect( render(base).html()).toEqual('<b>important</b>');
  expect(shallow(base).html()).toEqual('<div><b>important</b></div>');
});

Which is... very confusing for someone getting started with Enzyme, and adds a lot of extra sanity checks along the way 😢

IMHO, it should be either the equivalent of innerHTML or outerHTML, but not both at the same time! I prefer outerHTML since I think it makes more sense with the virtual DOM and abstract representation ("Give me the whole html for the current element"). However jQuery (hello, cheerio) uses innerHTML so that'd be fine as well. Just, the same for all methods.

@mykhailo-riabokon
Copy link
Contributor

mykhailo-riabokon commented May 14, 2018

@franciscop-invast If you are using jest as a test runner you have an option to make snapshots. It gives you more flexible and clean way to test your HTML output. I know that it does not solve this issue but it's an alternative way to do the same thing.

@franciscop-invast
Copy link

franciscop-invast commented May 14, 2018

I am actually not rendering the .html() for testing, just for debugging purposes, and was really confused with this. My main issue though was the difference between .shallow() and .render() in the way that .shallow() will only put the components, while .render() will show the underlying html (which is what I wanted). But trying to use .html() to debug why my selectors were not found ({ length: 0 }) was made extra difficult due to these inconsistencies.

Thanks for the tip @mykhailo-riabokon , but the snapshots seems very strict while I'm only interested in testing few things, so Enzyme works better for me (thanks for making it!).

@mykhailo-riabokon
Copy link
Contributor

@franciscop-invast I think .debug() method should have worked better for this case, as .html() purpose is a bit different.

And as for snapshot testing, I have used it for a while and it's not strict at all :) And if you want to scale your assertions in one day (I mean test a bit more than a few things) you already have a solution in place.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2018

enzyme doesn't support snapshots, but .debug() is a far superior method for doing that than .html() is, unrelated to the differences documented in this thread.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2018

I'm going to close this - however if anyone in the future can provide a PR that would address the inconsistencies with render vs shallow/mount, I'd be happy to review it.

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

9 participants