Use "undefined" instead of null

[tomitrescak]

Hi, when the ternary operator is created with the null a ? <b/> : null it creates havoc with some react approaches, such as with hot reloading or with react tools. Is it possible to somehow redefine the structure of the generated ternary statement to use undefined or use && if no else exists?

This seems to work fine: a ? <b/> : undefined

Examples of problems:

• Uncaught TypeError: Invalid value used as weak map key @backend.js 525 facebook/react-devtools#198
• TypeError: Invalid value used as weak map key(…) gadicc/meteor-hmr#99

[tomitrescak]

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);
  }

[AlexGilleran]

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? :)

[jimfb]

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.

[tomitrescak]

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.

[AlexGilleran]

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.

[tomitrescak]

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.

[texttechne]

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.

[AlexGilleran]

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

facebook/react-devtools#198