search

AlexGilleran / jsx-control-statements

Neater If and For for React JSX
https://www.npmjs.com/package/babel-plugin-jsx-control-statements
MIT License
1.62k stars 64 forks source link

Issue with Coverage #75

Closed OverseePublic closed 6 years ago

OverseePublic commented 6 years ago

https://github.com/istanbuljs/nyc/issues/812

texttechne commented 6 years ago

Works as expected and the coverage report is correct. You hard coded true and false in the <If> component. Since this plugin only transpiles the "component" to a ternary expression <If condition={true} is the same as true ? "OK" : null.

So the else branch is never checked. No miracle there, everything works as expected: Istanbul and jsx-control-statements. Your test code is flawed if you expect 100% coverage of such a piece of code.

texttechne commented 6 years ago

@OverseePublic by the way, thanks for referencing this in the React repository, warning about this plugin and stating that it is buggy:

A lot of fuss for an incorrect understanding of your coverage report...

And your time would've been better spent if you would have taken a look at our test cases and coverage reports before filing those issues.

OverseePublic commented 6 years ago

I think you need to read again. I do not plan to waste more time. This will be my last comment for you.

‘’’ There are thousands of React libraries that do strange things; we can't warn about every single one of them. Whenever this has been asked, we have always recommended against trying to recreate control flow as React components. - Dan ‘’’

texttechne commented 6 years ago

Sorry for the rude tone, but I was pissed off because of the wrong statements you've made everywhere.

Anyways, citing an authority like Dan Abramov with a general statement doesn't make your statements true: Your test is still flawed not JCS.

And Dan is completly right, it is not a good idea to "reacreate control flow as React components". However, this is a Babel plugin and not a React component. See here for explanations: https://github.com/AlexGilleran/jsx-control-statements#a-note-on-transformation-and-alternative-solutions

AlexGilleran commented 6 years ago

I think there's a bit of a hangover here because when we originally made this, React was pretty new and crazy and using JSX at all (HTML in your javascript? What about MVC?!?!) was a cowboy move. Nearly nothing worked with it, you were lucky to even get a text editor to highlight it properly. Getting it to even compile resulted in copy-pasting a bunch of webpack or browserify stuff you didn't understand and given the churn back then you fully expected to be using an entirely different javascript framework in 6 months anyway.

Years later the ecosystem has matured and grown and people have very different expectations around interoperability - now you can just clone create-react-app and go, you don't even need to know what webpack or babel does in order to make an app. I guess when I see the words "babel plugin" I tend to think that implies "this is probably going to break something else so be careful", but other people expect everything to just work? Maybe the start of the readme needs to make this a bit clearer.

Or maybe we just need to quote https://github.com/facebook/react/issues/12698:

ya'll free to do whatever works for your team.

:p

AlexGilleran commented 6 years ago

OK I've added a ☠️ warning ☠️ to the Readme, so no one can say I didn't warn them now :p.

I think the middle ground between ugly control flow and having everything work is probably doing this with render props instead of a babel plugin, which I started over here but haven't had more time for :(.