Smarter error messages for JSX accidentally put outside a JSX expression

justingrant commented 5 years ago

Search Terms

TS2657 TS2695 TS2304 JSX map label:"Domain: JSX/TSX"

Suggestion

TS should emit clearer error messages for invalid JSX caused by the case where a developer expected code to be in a JSX expression context but instead it's evaluated in a non-JSX context (regular code). A signpost for these kinds of issues is that the compiler errors go away if the offending code is wrapped in a Fragment.

Use Cases

TS emits a helpful error message ([ts] JSX expressions must have one parent element. [2657]) when a simple JSX expression has two sibling elements without a single parent element or fragment. But this only works if your code successfully emits multiple JSX elements. If you're combining JSX and text e.g. <Foo>{bar} then the helpful error message is not shown.

These kinds of errors are easy to solve in simple JSX but can be really challenging to spot when the offending code is in the middle of complex TS code, because it's not obvious that the problem is JSX-related instead of "something in my TS code is broken".

The root cause is a misunderstanding between the developer (who's assuming code to be in a JSX expression context) and the compiler (which knows that the code is NOT in a JSX expression context.

From the developer's point of view, the fix for these problems is simple: wrap the offending TS code in a Fragment or another element. This gave me an idea for how TS could perhaps show smarter error messages. Could the TS compiler recognize cases where the following two conditions are true? 1) the developer may have expected code to be evaluated as a JSX expression, e.g. the code is inside a JSX element, or the code has a JSX element as a sibling. 2) the compiler errors go away if the code is treated as a JSX expression instead of as regular TS code

If so, then the TS compiler could show an error message (e.g. "JSX expressions must be wrapped in a fragment or JSX element") at the start or end of the offending code. Even if there are false positives-- e.g. legitimate code bugs inside a {code} block inside some JSX-- it's probably better to give devs multiple possible reasons for a compiler error than to omit a likely reason.

For example, here's two patterns I've run across recently:

Pattern A: An inline function is defined inside a JSX element (or fragment) AND and there's a compiler error in that code AND the errors go away if the function's body were wrapped in a fragment. Examples:

    Test #1: GOOD
    <>
      {data.map((str: string, i: number) => (
        {str} // no error
      ))}
    </>

    Test #2: GOOD
    <>{data.map((str: string, i: number) => (
        <>{i}: {str}</> // no error
      ))}
    </>

    Test #3: BAD
    <>
      {data.map((str: string, i: number) => (
        {i}: {str} // errors: [ts] ')' expected. [1005]; [ts] Cannot find name 'str'. [2304]
      ))}
    </>

Pattern B: A JSX element (wrapped in < and >) is immediately preceded and/or immediately followed by non-JSX code that generates a compiler error AND the code would not throw a compiler error if it were wrapped in a Fragment.

Test #4: GOOD 
<>
  {data.map((str: string, i: number) => (
    {i}+": "+{str}+"\n" // no error
  ))}
</>

Test #5: GOOD
<>
  {data.map((str: string, i: number) => (
    <br/><>{i}+": "+{str}+"\n"</> // error on <br/>: [ts] Left side of comma operator is unused and has no side effects. [2695]
  )) // Good error here: [ts] JSX expressions must have one parent element. [2657]
  }      
</>

Test #6: BAD
<>
  {data.map((str: string, i: number) => (
    <br/>{i}+": "+{str}+"\n" // errors: [ts] Cannot find name 'i'. [2304]; [ts] Cannot find name 'str'. [2304]
  )) // error (on second parenthesis): [ts] '}' expected. [1005]
  }      
</>

Examples

Here's the full example from above.

const repro = () => {

  const simpleJSX = (
    <div>hi</div>  // confusing error here: [ts] Left side of comma operator is unused and has no side effects. [2695]
    <div>bye</div>
  ); // Good error here: [ts] JSX expressions must have one parent element. [2657]

  const data = ['a', 'b', 'c'];
  return (
  <>
    Test #1: GOOD
    <>
      {data.map((str: string, i: number) => (
        {str} // no error
      ))}
    </>

    Test #2: GOOD
    <>{data.map((str: string, i: number) => (
        <>{i}: {str}</> // no error
      ))}
    </>

    Test #3: BAD
    <>
      {data.map((str: string, i: number) => (
        {i}: {str} // errors: [ts] ')' expected. [1005]; [ts] Cannot find name 'str'. [2304]
      ))}
    </>

    Test #4: GOOD 
    <>
      {data.map((str: string, i: number) => (
        {i}+": "+{str}+"\n" // no error
      ))}
    </>

    Test #5: GOOD
    <>
      {data.map((str: string, i: number) => (
        <br/><>{i}+": "+{str}+"\n"</> // error on <br/>: [ts] Left side of comma operator is unused and has no side effects. [2695]
      )) // Good error here: [ts] JSX expressions must have one parent element. [2657]
      }      
    </>

    Test #6: BAD
    <>
      {data.map((str: string, i: number) => (
        <br/>{i}+": "+{str}+"\n" // errors: [ts] Cannot find name 'i'. [2304]; [ts] Cannot find name 'str'. [2304]
      )) // error (on second parenthesis): [ts] '}' expected. [1005]
      }      
    </>

  </>
)};

Checklist

My suggestion meets these guidelines:

[x] This wouldn't be a breaking change in existing TypeScript/JavaScript code (because it only applies to code that already has a compiler error)
[x] This wouldn't change the runtime behavior of existing JavaScript code
[x] This could be implemented without emitting different JS based on the types of the expressions
[x] This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
[x] This feature would agree with the rest of TypeScript's Design Goals.

justingrant commented 5 years ago

For reference, here's the place in the compiler's parsing code that the "good" message TS2657 is generated: https://github.com/Microsoft/TypeScript/blob/d79c37cd191a5d10ad678f2a1379c6267e914bf2/src/compiler/parser.ts#L4020

danielbrauer commented 4 years ago

This is a very good suggestion, and @justingrant explains very well the mismatch between the compiler's understanding and my own.

I ended up here after being confused for the nth time by TS2695: Left side of comma operator is unused and has no side effects. But there is no comma within a mile of the offending fragment. Worse than simply not explaining the problem properly, TS2695 sends the user on a wild goose chase in this scenario.

Javascript currently gives the following error for the same scenario, which couldn't be clearer:

Parsing error: Adjacent JSX elements must be wrapped in an enclosing tag. Did you want a JSX fragment <>...</>?

jharris1993 commented 2 years ago

Note that this is not limited to JSX elements.

Given the following code fragment:

//  is_someting_happening is my attempt to create a joystick "event" when "something interesting" happens.
//  "someting interesting" = any joystick movement if either enabling trigger is pressed
//  or, if a previously pressed trigger has been released.
//  Otherwise, no data should be sent.

function is_something_happening(gopigo3_joystick) {
    if (gopigo3_joystick.trigger_1 == 1 || gopigo3_joystick.head_enable == 1) {  //  Has an enabling trigger event happened?
        if (old_time_stamp != Number.parseFloat((gopigo3_joystick.time_stamp).toFixed())) {  // and, has the timestamp changed?
            send_data(gopigo3_joystick)  //  then, send data to the robot and. . .
            old_time_stamp = Number.parseFloat((gopigo3_joystick.time_stamp).toFixed())  //  Save current time_stamp to compare to future changes
            old_trigger_state = 1  // record trigger was pressed
        }
    }
    else if (old_trigger_state == 1) {  // current trigger value MUST be zero here - old_trigger_state = 1 means a trigger has changed
        send_data(gopigo3_joystick)  // send the trigger release event
        old_time_stamp = gopigo3_joystick.time_stamp  //  Save current time_stamp to compare to future changes
        old_trigger_state = 0  // record the fact that the trigger was released
    }
    return(old_time_stamp, old_trigger_state);
}

Generates this error in Visual Studio Code: "Left side of comma operator is unused and has no side effects.

Note that the expression return(old_time_stamp, old_trigger_state); is actually wrong as JavaScript cannot return multiple discrete values from a function, (which I did not know until this happened), however the error message is less than clear and does not indicate to me the exact nature of the mistake; that is, trying to return multiple values from a JavaScript function call.

I suspect that this particular case falls outside the JSX element case defined by the original post, but it appears to be captured by the same logic which is confusing. If the suggested "parsing error" error message is implemented, (Adjacent JSX elements must be wrapped in an enclosing tag), it would be even less clear than it is now.

Thanks!

microsoft / TypeScript