Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extraneous prefix semicolon inside if-statement #1244

Closed
hawkrives opened this issue Apr 13, 2017 · 8 comments
Closed

Extraneous prefix semicolon inside if-statement #1244

hawkrives opened this issue Apr 13, 2017 · 8 comments
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken

Comments

@hawkrives
Copy link
Contributor

[online demo]

if (expr === 'boolean') {
  ({ x } = compute())
}

// becomes

if (expr === "boolean") {
  ;({ x } = compute())
}

I'd prefer it if the semicolon wasn't there? But I can probably get used to it and/or refactor this part of my code.

@bakkot
Copy link
Collaborator

bakkot commented Apr 13, 2017

This is on purpose.

@vjeux vjeux added the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label Apr 13, 2017
@lourd
Copy link

lourd commented Apr 17, 2017

I have an issue related to this, but it's additionally in the context of do expressions.

Reproduction

Before

<div>
  { do {
    if (isDashboardRoute(this.props.location)) {
      if (!this.props.loggedIn) {
        <Redirect to={{ pathname: '/login', state: { from: this.props.location } }} />
      } else {
        <Dashboard key="d" />
      }
    } else if (isAccountsRoute(this.props.location)) {
      <Accounts key="a" />
    } else if (this.props.location.pathname === '/console') {
      <Route
        key="c"
        path="/console"
        component={Console}
        restrictTo={Permissions.LoggedIn}
      />
    } else {
      <Redirect to="/login" />
    }
  }}
</div>

After

<div>
  {
    do {
      if (isDashboardRoute(this.props.location)) {
        if (!this.props.loggedIn) {
          ;<Redirect
            to={{ pathname: "/login", state: { from: this.props.location } }}
          />
        } else {
          ;<Dashboard key="d" />
        }
      } else if (isAccountsRoute(this.props.location)) {
        ;<Accounts key="a" />
      } else if (this.props.location.pathname === "/console") {
        ;<Route
          key="c"
          path="/console"
          component={Console}
          restrictTo={Permissions.LoggedIn}
        />
      } else {
        ;<Redirect to="/login" />
      }
    }
  }
</div>

I would rather the leading semicolons not be there. The prettified code still works as before, after transpiled with the transform-do-expressions Babel plugin, but there is a slightly different output.

For reference, here's the Babel transform before, and here's the Babel transform after.

@bakkot
Copy link
Collaborator

bakkot commented Apr 17, 2017

@lourd, that's on purpose for the same reason as the parentheses case. In particular, if the previous line is an expression, it won't parse without the semicolon:

if (!this.props.loggedIn) {
  console.log('debug')
  <Redirect // SyntaxError
    to={{ pathname: "/login", state: { from: this.props.location } }}
  />
}

I don't think it makes sense to put it there for the ( and not the <, and I think a majority of the no-semi people want in there for ( for the reason linked above.

This is the price you pay for the no-semi style.


Edit: not sure why Babel has different output, but the two untransformed programs definitely have the same semantics; you might consider raising the issue with them, if it bothers you.

@lourd
Copy link

lourd commented Apr 17, 2017

Hey @bakkot, thanks for your quick response. I've played around with some more expressions and understand do statements a little better now. I agree, looks like I'll just have to deal if I want semi-colon free style.

I am confused about the difference in these two cases — a do expression within JSX, versus plain variable assignment. The variable assignment case runs without semi colons, either after the log expressions or before the numbers, but the JSX case does not.

let a = 2
let b = do {
  if (a) {
    console.log('a is', a)
    4
    console.log('funny joke here')
    6
  }
  else 5
}
console.log('b is', b)

let c = 
  <div>
  {
    do {
      if (true) {
        console.log('hmm?')
        // This fails
        // <span>Hmm</span>
        // This works
        ;<span>Hmm</span>
      } else {
        console.log('whaaa')
        // Fails
        // <span>Whaaa</span>
        // Works
        ;<span>Whaaa</span>
      }
    }
  }
  </div>

// console output
/*
"a is" 2
"funny joke here"
"b is" 6
"hmm?"
*/

@bakkot
Copy link
Collaborator

bakkot commented Apr 17, 2017

@lourd:

So, a bit about the JavaScript grammar. It technically requires semicolons at the end of most statements. You can get away without them in some cases through what's called Automatic Semicolon Insertion, or ASI, which basically amounts to "if a line ends without a semicolon, but there's no way to parse the start of the next line as a continuation of the current one, the parser is to pretend there was a semicolon there".

console.log('a is', a)
4

parses because there's no way to consider the 4 as a continuation of the console.log('a is', a): console.log('a is', a)4 is not the start of any valid JavaScript statement. So the parser performs ASI before the 4. (But if instead you had (4), it could be: console.log('a is', a)(4) is syntactically valid, even if it doesn't work at runtime because console.log doesn't return a function, so you'd need to write a semicolon there.)

Same deal when followed by a variable assignment: console.log('a is', a)let a isn't valid syntax, so the parser inserts a semicolon before the let.

Conversely, f()<bar is valid syntax, so the parser does not perform ASI in

console.log('debug')
<Redirect

and it doesn't parse as you intended (and then later chokes on the {{).

@lourd
Copy link

lourd commented Apr 18, 2017

Thank you for your articulate answer @bakkot.

I am confused about your second example. f()<bar is really f()React.createElement('bar', ..., which is not valid syntax, so would be a case where the semicolon is inserted.

let b = do {
  if (true) {
    console.log('hmm')
    <span>Haaa</span>
    // Works
    // ;<span>Haa</span>
  }
}

let c = do {
  if (true) {
    console.log('haa')
    React.createElement('span', null, 'haa')
  }
}

Playing around with this in Babel confirms your reasoning — the former example fails unless there's a semicolon. But, they both convert to the same thing.

Is a JSX node considered an expression, where the converted code is not?

@bakkot
Copy link
Collaborator

bakkot commented Apr 18, 2017

@lourd, it has to be parsed before it can be translated to React.createElement. That translation happens way down the line; at this stage it's just reading the raw characters you wrote and trying to make sense of them.

@rattrayalex
Copy link
Collaborator

Closing in favor of #1433 , which specifically addresses the JSX case.

As mentioned above, this is is otherwise behaving as intended.

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken
Projects
None yet
Development

No branches or pull requests

5 participants