Use "undefined" instead of null about jsx-control-statements HOT 8 CLOSED

In ast.js I changed

if (!blocks.length) {
    return babelTypes.NullLiteral();
  }

to

if (!blocks.length) {
    return babelTypes.BooleanLiteral(false);
  }

And all works in hot module reload AND in react tools.
I think there is no harm in returning false instead of null.

Would you consider adding this to the core? Or at least somehow have the possibility to specify which literal can be returned as a "null" literal.

[EDIT] Same needs to be updated in choose statement:

  if (!result[ELEMENTS.OTHERWISE]) {
    result[ELEMENTS.OTHERWISE] = types.BooleanLiteral(false);
  }

from jsx-control-statements.

Ah man, we only just changed to to null in #17!

Returning false appears to be fine according to https://facebook.github.io/react/docs/component-specs.html so I guess we can do it. Weird that null breaks it though.

Can you make a PR with what you've done? :)

from jsx-control-statements.

Returning null should work, as per the spec. If there are situations where null breaks something, that should be fixed upstream. Returning null seems like the best thing to do. Returning undefined would also be fine. Returning false is slightly less good, but could work.

Honestly, I would just close this as "won't fix", and ask people to file bugs on the libraries/tools. jsx-control-statements is completely within spec on this point, afaik. Playing whack-a-mole with bugs in other systems is not a sustainable solution.

from jsx-control-statements.

null breaks it in several different packages ... if changing it to false or undefined will have only positive effects, not sure why it should not be fixed? I'll prepare PR soon, am currently on my way to Europe, so I'll do that ASAP.

from jsx-control-statements.

Depends if changing it to false fixes this and creates some kind of equivalent obscure problem in something else ;).

What @jimfb says makes sense but I think we should at least support a fork until the underlying issue is fixed.

from jsx-control-statements.

I'm open to anything, am running from my fork anyways, I only asked if you would consider bringing this into the core. If not, it's perfectly fine and I'll just update from git to latest version when needed.

from jsx-control-statements.

Switching to undefined would be fine IMHO. But @tomitrescak are you sure that this will fix the issue? As far as I can tell from the mentioned issues this wasn't a proposed solution. So have you tested this approach?

I would really consider doing this, since in my experience such nasty browser bugs get rarely fixed by browser maintainers.

from jsx-control-statements.

A fix for this got merged into React master 11 days ago 🎆

facebook/react-devtools#198

from jsx-control-statements.