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

Edge case with empty elements #6

Closed
MoOx opened this issue Nov 2, 2015 · 18 comments
Closed

Edge case with empty elements #6

MoOx opened this issue Nov 2, 2015 · 18 comments
Labels

Comments

@MoOx
Copy link

MoOx commented Nov 2, 2015

I am using tape-jsx-equals and have the following code

  renderer.render(
    <Picture />
  )
  t.jsxEquals(
    renderer.getRenderOutput(),
    <div></div>,
    "should render nothing if nothing is passed as props"
  )

(I only pasted the relevant part of my code)

The issue I get is:

    expected: '<div />'
    actual:   '<div>\n  \n</div>'

The expected result I wrote is <div></div>, but somehow it got transformed to <div />. Maybe by babel.

Maybe <div>{whitespace}</div> should be rendered to <div />?

@vvo
Copy link
Contributor

vvo commented Nov 2, 2015

Do you think you can write a test and submit it? Adding a new test is REALLY easy.

Thanks for the bur report thought

@MoOx
Copy link
Author

MoOx commented Nov 2, 2015

Done in #7
I think both tests in this section should works. But maybe I am wrong :)

@vvo
Copy link
Contributor

vvo commented Nov 2, 2015

both tests

Both? I see one test

@MoOx
Copy link
Author

MoOx commented Nov 2, 2015

The one added and the one just before it.

@vvo
Copy link
Contributor

vvo commented Nov 2, 2015

Strangely your actual failing test in the issue description should work. But what you added can't. I chose to have a consistant resulting string because in the end when you compare JSX you won't have this issue.

The added test case is not the one we are looking for I think.

I will create a custom component rendering a <div></div> and see how it goes.

@vvo
Copy link
Contributor

vvo commented Nov 2, 2015

Maybe

{whitespace}
should be rendered to
?

So if you picture element is inserting line breaks inside the <div></div> then you need to assert for it because that's not the same thing as an empty <div/>.

I am the only transforming <div></div> (JSX) to <div /> for ease of comparing strings and because I cannot know from the React element that it was written as <div></div> or <div />.

But in your case, you do have line breaks in the render() so you should test for it. You could also implement toIncludeJSX as I did in expect-jsx: https://github.com/algolia/expect-jsx/blob/master/index.js#L22

It should remove the line breaks, not sure thought

@vvo
Copy link
Contributor

vvo commented Nov 2, 2015

Maybe we could add an option to remove line breaks/whitespace from empty elements, happy to take a PR on this but unsure this is the right move (and it would be an option).

@vvo vvo added question and removed unconfirmed labels Nov 2, 2015
@MoOx
Copy link
Author

MoOx commented Nov 3, 2015

The thing is: my <Picture /> with no props was rendering a <div><div/>

  render() {
    return (
      <div>
       {
         /* falsy thing */
       }
      </div>
    )
  }

@vvo
Copy link
Contributor

vvo commented Nov 3, 2015

But in an html page, does it renders as <div></div> or <div>\n \n<div>?

@vvo
Copy link
Contributor

vvo commented Nov 3, 2015

It seems babel transform thinks it's the same to write <div></div> or <div>\n \n</div> so we might have to do something here yes

@vvo
Copy link
Contributor

vvo commented Nov 3, 2015

So both line breaks and whitespace are working BUT

<div>{false}</div>

Is outputed as <div>\n\n</div>. I wonder if we need to fix this, there's still a difference between <div>{false}</div> and <div></div>

@vvo
Copy link
Contributor

vvo commented Nov 3, 2015

In your case it should be null, not false, this would then make your test pass.

@MoOx
Copy link
Author

MoOx commented Nov 6, 2015

imo, we should not consider dfferences between <div>{false}</div> and <div></div>. This module is generating a string, and the {false} thing should not be take into account.

@vvo
Copy link
Contributor

vvo commented Nov 7, 2015

Really what worries me is that there IS a difference: https://babeljs.io/repl/#?experimental=true&evaluate=true&loose=false&spec=false&code=var%20div%20%3D%20%3Cdiv%3E%7Bfalse%7D%3C%2Fdiv%3E%3B%0Avar%20div2%20%3D%20%3Cdiv%3E%7Bnull%7D%3C%2Fdiv%3E%3B

I did not stumble on this usecase (wanting to assert {false} and ignoring whitespace). I would say that if there's nothing to display then maybe return null or nothing at all in your library.

Otherwise happy to take a PR solving the {false} sole element case in a "nice" way. I am so worried that the first tricky PR comes from you, you could have been a good supporter but now I have the feeling the lib looks weird to you :) :D

@vvo
Copy link
Contributor

vvo commented Nov 8, 2015

fixed and released expect-jsx@2.1.1

npm install expect-jsx@2.1.1 --save-dev

@vvo
Copy link
Contributor

vvo commented Nov 8, 2015

Hmm seems like I will have to revert, weird edge cases again

@vvo
Copy link
Contributor

vvo commented Nov 8, 2015

ok we should be good here now

@MoOx
Copy link
Author

MoOx commented Nov 10, 2015

Thanks for you work. I will take a look to your fix when I will be back on my jsx test :)

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

No branches or pull requests

2 participants