babel / minify

:scissors: An ES6+ aware minifier based on the Babel toolchain (beta)
https://babeljs.io/repl
MIT License
4.39k stars 225 forks source link

dead-code-elimination erroneously tries to inline expressions into JSXIdentifiers. #724

Open tolmasky opened 7 years ago

tolmasky commented 7 years ago

Reproduction on RunKit: https://runkit.com/tolmasky/dead-code-inlines-jsxidentifiers

function Input(element)
{
    var ElementType = element || "input";

    return <ElementType ref = "input" />;
}

This will try to put (element || "input") into the actual JSX element, (something like <(element||"input") ref = "input/>), which is not allowed, and will thus throw this error:

TypeError: unknown: Property name of JSXOpeningElement expected node to be of a type ["JSXIdentifier","JSXMemberExpression"] but instead got "LogicalExpression"

It is important that I run this before my actual React transforms, since there are in-between transforms that rely on the right code having been removed, so I can not use as a temporary workaround switching the order of the transformations.

boopathi commented 7 years ago

The minifier understands JS and outputs JS. I'm not sure this is a useful feature for the minifier to understand JSX and have workarounds for the same. I'm closing this issue and the PR linked as won't fix.

hzoo commented 7 years ago

I believe this is because @tolmasky actually is using jsx as code instead of for react? yeah hard to say since it does feel wrong to add all these workarounds everywhere (unless we could make that generic too)

boopathi commented 7 years ago

Maybe, we can look at the path to be replaced and from babel-types we can determine if it is an expression of any type or restricted to only - say, Identifier or expression of particular types, etc...

tolmasky commented 7 years ago

I would be certainly happy to replace:

if (!refPath.isJSXIdentifier()) continue;

with:

if (!refPath.isExpression()) continue;

I believe this is indeed the most generic way of explaining the what is going on -- only swap in an identifier if the call site itself accepts an expression.

However, it seems that babel-types currently lists JSXIdentifier as an Expression. I believe this is a bug in babel-types, as JSXIdentifiers certainly cannot appear anywhere an Expression can appear in the JS grammar, unlike JSXElement which can and is thus fine to be listed as an Expression. I could go submit a quick PR to babel-types to get that changed and then make the non-JSX requiring PR here. Does that sound reasonable?

boopathi commented 6 years ago

Using isExpression sounds good

Aside: From the JSX Spec

JSXElementName :
  JSXIdentifier
  JSXNamespacedName
  JSXMemberExpression

I could go submit a quick PR to babel-types to get that changed and then make the non-JSX requiring PR here. Does that sound reasonable?

👍 Sure. Thanks.