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

Optional use of semicolons #825

Open
majjoha opened this issue Sep 19, 2015 · 12 comments
Open

Optional use of semicolons #825

majjoha opened this issue Sep 19, 2015 · 12 comments
Labels
bug

Comments

@majjoha
Copy link
Contributor

@majjoha majjoha commented Sep 19, 2015

I couldn't find an existing issue about this, but assume I have the following file:

/* @flow */
class Foo {
  bar: string;
  baz: number;

  constructor(bar: string, baz: number) {
    this.bar = bar
    this.baz = baz
  }
}

Would it be possible to make the semicolon optional in the annotated fields, so the code could be this instead?

/* @flow */
class Foo {
  bar: string
  baz: number

  constructor(bar: string, baz: number) {
    this.bar = bar
    this.baz = baz
  }
}
@cesarandreu
Copy link
Contributor

@cesarandreu cesarandreu commented Sep 19, 2015

+1

Babel can parse it, but flow gives you an error :(. It's also incredibly confusing for certain things inside the class body to have semicolons and others to not have em. I prefer to just drop em all and never start a line with [ or (.

@Gozala
Copy link
Contributor

@Gozala Gozala commented Sep 22, 2015

+1

2 similar comments
@Jabher
Copy link
Contributor

@Jabher Jabher commented Oct 27, 2015

+1

@chris-lock
Copy link

@chris-lock chris-lock commented Nov 9, 2015

+1

@jeffmo
Copy link
Member

@jeffmo jeffmo commented Nov 9, 2015

Note that Flow is just following the ES proposal being iterated on here: https://github.com/jeffmo/es-class-properties (so it's probably best to continue this discussion there if there are further thoughts/concerns).

But to sum things up, unfortunately the grammar for class field definitions do require a semicolon mainly because of computed property names:

Computed names in methods:

class Foo {
  a = b [computedName] () {}
}

To support this would require unbounded lookahead in a parser. Such a thing is something the language has always rejected in the past due to performance concerns in implementations, so there's little to no chance of this passing TC39's acceptance.

Computed names in other properties:

class Foo {
  a = b [computedName] = 42
}

Should this mean a = b; [computedName] = 42;? Or does it mean a = b[computedName] = 42;? This one is actually completely ambiguous :/

@ntucker
Copy link

@ntucker ntucker commented Jun 1, 2016

class Foo {
a = b [computedName] = 42
}

Should this mean a = b; [computedName] = 42;? Or does it mean a = b[computedName] = 42;? This one is actually completely ambiguous :/

It's actually not. In javascript it means a = b[computedName] = 42; Endlines are what can be substituted for semicolons, not any arbitrary point. The rules of javascript are quite simple and established, and flow should support javascript if it wants to be used as such.

Some educational material: http://inimino.org/~inimino/blog/javascript_semicolons

@jurca
Copy link

@jurca jurca commented Jan 2, 2017

@jeffmo The proposal actually does not require us to use semicolons as it relies on ASI; that has been a misunderstanding that has been cleared up here:
tc39/proposal-class-public-fields#26

and here:
tc39/proposal-class-public-fields#26 (comment) (by yourself, actually ;) )

I see that this is implemented for classes correctly, however interfaces do require semicolons for some reason.

Therefore I suggest re-opening this issue and fixing this.

@grantgeorge
Copy link

@grantgeorge grantgeorge commented Mar 7, 2017

+1

@nmn
Copy link
Contributor

@nmn nmn commented Mar 7, 2017

@jurca

So, the issue is resolved for classes.

In the case of interfaces, you need to use commas or semi-colons like you do for any object type.
However, I think that's a parser a bug. (Typescript treats interfaces like classes for parsing).

@nmn nmn reopened this Mar 7, 2017
@calebmer calebmer added the bug label Jul 19, 2017
@motiz88
Copy link
Contributor

@motiz88 motiz88 commented Sep 25, 2017

It appears that the parser currently inserts semicolons over-eagerly in class bodies, specifically by not looking for newlines but just treating semicolons as optional. The following parses but shouldn't, IMHO (and it doesn't in Babylon).

class A {
  foo: boolean get
  bar(): void {
  }
}

Relevant work in Babylon: babel/babylon#351.

@zaynv
Copy link

@zaynv zaynv commented Feb 23, 2018

is this issue the same as this one? if so, i will close mine

@ntucker
Copy link

@ntucker ntucker commented Feb 27, 2018

@zaynv seems to be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.