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

Babel v6.4 Requires semicolons after class properties #372

Closed
cableray opened this Issue Jan 6, 2016 · 33 comments

Comments

@cableray

cableray commented Jan 6, 2016

See babel/babel#3225. Seems like this is an incompatible change with standard. Anyone aware of this?

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 7, 2016

Member

If the JS specification defines that a semicolon must be after a class property, then, well, we'll have to adapt to allow that rule.
If it doesn't, then I suspect its just a matter of time until Babel can adapt or we'll have to make an exception.

@kittens? 🎱 Haha

Member

dcousens commented Jan 7, 2016

If the JS specification defines that a semicolon must be after a class property, then, well, we'll have to adapt to allow that rule.
If it doesn't, then I suspect its just a matter of time until Babel can adapt or we'll have to make an exception.

@kittens? 🎱 Haha

@dcousens dcousens added the question label Jan 7, 2016

@bcomnes

This comment has been minimized.

Show comment
Hide comment
@bcomnes

bcomnes Jan 7, 2016

Member

Urg, gross. So ASI won't work inside of classes?

Member

bcomnes commented Jan 7, 2016

Urg, gross. So ASI won't work inside of classes?

@LinusU

This comment has been minimized.

Show comment
Hide comment
@LinusU

LinusU Jan 7, 2016

Contributor

I think that babel was a bit quick to jump the gun here, but on the other hand they are targeting an unfinished spec so I guess that that is expected.

@feross I think that your name has some weight in the javascript community and I would love for you to chime in with your thoughts over at tc39/proposal-class-public-fields#26

I think that they have a very bad rationale for requiring the semicolons, and I still think that there is time to change their minds about this. It seems very strange to require semicolons in this particular place, when they aren't needed elsewhere.

Contributor

LinusU commented Jan 7, 2016

I think that babel was a bit quick to jump the gun here, but on the other hand they are targeting an unfinished spec so I guess that that is expected.

@feross I think that your name has some weight in the javascript community and I would love for you to chime in with your thoughts over at tc39/proposal-class-public-fields#26

I think that they have a very bad rationale for requiring the semicolons, and I still think that there is time to change their minds about this. It seems very strange to require semicolons in this particular place, when they aren't needed elsewhere.

@flying-sheep

This comment has been minimized.

Show comment
Hide comment
@flying-sheep

flying-sheep Jan 7, 2016

i think it’s simply a quick&dirty rule to have something backwards-compatible. (you can always remove a semicolon made redundant by ASI, this way introducing ASI will never generate a syntax error)

well, unless the definition gets changed to be similar to method literals which may not have semicolons (AFAIK)

i think medium-to-long term there will be intuitive elision rules for class properties, we just have to keep vigilant so that this doesn’t accidentally end up in some standards document

flying-sheep commented Jan 7, 2016

i think it’s simply a quick&dirty rule to have something backwards-compatible. (you can always remove a semicolon made redundant by ASI, this way introducing ASI will never generate a syntax error)

well, unless the definition gets changed to be similar to method literals which may not have semicolons (AFAIK)

i think medium-to-long term there will be intuitive elision rules for class properties, we just have to keep vigilant so that this doesn’t accidentally end up in some standards document

@ThaJay

This comment has been minimized.

Show comment
Hide comment
@ThaJay

ThaJay Jan 7, 2016

It just cost me way too much time to figure out what had changed after trying to deploy my app on our server.
pretty lame we have to find out through a git isse. it's really hard to find this way. A blog post would have been nice, as I'm sure I am not the only one who has a problem with this (even if this change complies to the standard in it's current state).

It would have been logical to include configuration to make this behavior optional, there is nothing breaking about not wanting to comply to this detail.

It's actually flagging all my arrow functions that contain 'this', so now I have to refactor those back to normal functions and manually bind 'this'.

ThaJay commented Jan 7, 2016

It just cost me way too much time to figure out what had changed after trying to deploy my app on our server.
pretty lame we have to find out through a git isse. it's really hard to find this way. A blog post would have been nice, as I'm sure I am not the only one who has a problem with this (even if this change complies to the standard in it's current state).

It would have been logical to include configuration to make this behavior optional, there is nothing breaking about not wanting to comply to this detail.

It's actually flagging all my arrow functions that contain 'this', so now I have to refactor those back to normal functions and manually bind 'this'.

CookPete added a commit to CookPete/react-player that referenced this issue Jan 7, 2016

Use shrinkwrap to limit babel packages to 6.3
To circumvent the slightly lame new requirement for semicolons after class properties
standard/standard#372

rmehner added a commit to eHealthAfrica/grunt-tx that referenced this issue Jan 7, 2016

@cableray

This comment has been minimized.

Show comment
Hide comment
@cableray

cableray Jan 7, 2016

@ThaJay It was also in the release notes, but kind of hidden in there.

cableray commented Jan 7, 2016

@ThaJay It was also in the release notes, but kind of hidden in there.

@GantMan

This comment has been minimized.

Show comment
Hide comment
@GantMan

GantMan Jan 8, 2016

btw. I just merged a 56 file update in our "Standard JS badged" project. Where 56 semicolons were added. I can't imagine many people are happy with this.

GantMan commented Jan 8, 2016

btw. I just merged a 56 file update in our "Standard JS badged" project. Where 56 semicolons were added. I can't imagine many people are happy with this.

@thejmazz

This comment has been minimized.

Show comment
Hide comment
@thejmazz

thejmazz Jan 8, 2016

ugh, now won't all my babel ^6.x.x projects break when I clone and npm install them? I guess this what you get for playing with stage 1 syntax..

thejmazz commented Jan 8, 2016

ugh, now won't all my babel ^6.x.x projects break when I clone and npm install them? I guess this what you get for playing with stage 1 syntax..

@torifat

This comment has been minimized.

Show comment
Hide comment
@torifat

torifat Jan 8, 2016

You can use this codemod to upgrade your code.

https://gist.github.com/torifat/83da48336867e128f2ca

export default function(file, api) {
    const j = api.jscodeshift;
    const { expression, statement, statements } = j.template;

    return j(file.source)
        .find(j.ClassProperty)
        .replaceWith(p => {
          p.node.end++;
          return p.node;
        })
        .toSource();
};
$ jscodeshift -t ../codemod/static-property-semicolon.js src/

torifat commented Jan 8, 2016

You can use this codemod to upgrade your code.

https://gist.github.com/torifat/83da48336867e128f2ca

export default function(file, api) {
    const j = api.jscodeshift;
    const { expression, statement, statements } = j.template;

    return j(file.source)
        .find(j.ClassProperty)
        .replaceWith(p => {
          p.node.end++;
          return p.node;
        })
        .toSource();
};
$ jscodeshift -t ../codemod/static-property-semicolon.js src/
@GantMan

This comment has been minimized.

Show comment
Hide comment
@GantMan

GantMan Jan 8, 2016

We've found that if you add proptypes from outside the class, it won't require a semicolon.

e.g.

class Showing extends Scene {
...
} // close class

Showing.propTypes = {
  hands:  React.PropTypes.jazz
}
// No semicolons!

BONUS NOTE - My coloration was off for everything that followed proptypes (ES6 color in Sublime) and now that it can't have anything follow it, because it's not inside the class, the coloration issue is fixed, too.

GantMan commented Jan 8, 2016

We've found that if you add proptypes from outside the class, it won't require a semicolon.

e.g.

class Showing extends Scene {
...
} // close class

Showing.propTypes = {
  hands:  React.PropTypes.jazz
}
// No semicolons!

BONUS NOTE - My coloration was off for everything that followed proptypes (ES6 color in Sublime) and now that it can't have anything follow it, because it's not inside the class, the coloration issue is fixed, too.

@rstacruz

This comment has been minimized.

Show comment
Hide comment
@rstacruz

rstacruz Jan 8, 2016

Contributor

ugh, now won't all my babel ^6.x.x projects break when I clone and npm install them? I guess this what you get for playing with stage 1 syntax..

I recommend using --save-exact next time. I've been doing that for all my projects, and greenkeeper.io makes it easy to keep up.

Contributor

rstacruz commented Jan 8, 2016

ugh, now won't all my babel ^6.x.x projects break when I clone and npm install them? I guess this what you get for playing with stage 1 syntax..

I recommend using --save-exact next time. I've been doing that for all my projects, and greenkeeper.io makes it easy to keep up.

@LinusU

This comment has been minimized.

Show comment
Hide comment
@LinusU

LinusU Jan 8, 2016

Contributor

@rstacruz Isn't it better to use npm shrinkwrap?

Contributor

LinusU commented Jan 8, 2016

@rstacruz Isn't it better to use npm shrinkwrap?

@CookPete

This comment has been minimized.

Show comment
Hide comment
@CookPete

CookPete Jan 8, 2016

What I've done for the time being is use a partial shrinkwrap to pin babel-core, babel-cli and babylon to 6.3.x, which temporarily solves the issue: CookPete/react-player@630072c. It's not ideal but it prevents the build error for now.

It sounds like eventually we will have to succumb to the semicolon, if it is part of the official proposal.

CookPete commented Jan 8, 2016

What I've done for the time being is use a partial shrinkwrap to pin babel-core, babel-cli and babylon to 6.3.x, which temporarily solves the issue: CookPete/react-player@630072c. It's not ideal but it prevents the build error for now.

It sounds like eventually we will have to succumb to the semicolon, if it is part of the official proposal.

@jprichardson

This comment has been minimized.

Show comment
Hide comment
@jprichardson

jprichardson Jan 8, 2016

Member

It sounds like eventually we will have to succumb to the semicolon, if it is part of the official proposal.

Can anyone point out where it's part of the official proposal?

Member

jprichardson commented Jan 8, 2016

It sounds like eventually we will have to succumb to the semicolon, if it is part of the official proposal.

Can anyone point out where it's part of the official proposal?

@LinusU

This comment has been minimized.

Show comment
Hide comment
@LinusU

LinusU Jan 8, 2016

Contributor

@jprichardson I haven't seen it explicitly in the actual spec, but the author clarified that semicolons was indeed mandatory in this issue: tc39/proposal-class-public-fields#25. There is also a follow up discussion here: tc39/proposal-class-public-fields#26

@rstacruz Actually, that wouldn't have helped at all since the breaking change is in babylon which is a dependency of babel. npm shrinkwrap would have worked though...

Contributor

LinusU commented Jan 8, 2016

@jprichardson I haven't seen it explicitly in the actual spec, but the author clarified that semicolons was indeed mandatory in this issue: tc39/proposal-class-public-fields#25. There is also a follow up discussion here: tc39/proposal-class-public-fields#26

@rstacruz Actually, that wouldn't have helped at all since the breaking change is in babylon which is a dependency of babel. npm shrinkwrap would have worked though...

@bcomnes

This comment has been minimized.

Show comment
Hide comment
@bcomnes

bcomnes Jan 9, 2016

Member

It sounds like there may have been a misunderstanding with the requirement on babel's end (eg the semicolin requirement is still fulfilled by ASI and this babel behavior is a bug, except where it doesn't already with certain literals)... but still waiting for official word.

Member

bcomnes commented Jan 9, 2016

It sounds like there may have been a misunderstanding with the requirement on babel's end (eg the semicolin requirement is still fulfilled by ASI and this babel behavior is a bug, except where it doesn't already with certain literals)... but still waiting for official word.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 9, 2016

Member

@LinusU he actually goes on to say tc39/proposal-class-public-fields#25 (comment):

Yea, I think you might be right @jmm. I went in with this restriction with the mindset of creating an all-cases-work grammar, but I suppose if we don't require semicolons then only some cases don't work (and an optional semicolon becomes required only in those cases).
I will follow up here next week and plan to give an update on this at the coming TC39 meeting at the end of January.

That is, a return to the status quo of allowing ASI is still possible, but obviously up for discussion.

Member

dcousens commented Jan 9, 2016

@LinusU he actually goes on to say tc39/proposal-class-public-fields#25 (comment):

Yea, I think you might be right @jmm. I went in with this restriction with the mindset of creating an all-cases-work grammar, but I suppose if we don't require semicolons then only some cases don't work (and an optional semicolon becomes required only in those cases).
I will follow up here next week and plan to give an update on this at the coming TC39 meeting at the end of January.

That is, a return to the status quo of allowing ASI is still possible, but obviously up for discussion.

@niftylettuce

This comment has been minimized.

Show comment
Hide comment

niftylettuce commented Jan 10, 2016

@torifat thanks for this #372 (comment)

@favorit

This comment has been minimized.

Show comment
Hide comment
@favorit

favorit commented Jan 12, 2016

@torifat thanks!

@monfera

This comment has been minimized.

Show comment
Hide comment
@monfera

monfera Jan 14, 2016

Suddenly mandating the semicolons without a quick 'whoa, hang on a sec' option is throwing some obstacles. It was a surprise after npm install as I need to deploy code under time constraint, i.e. no time for MSI, and even fixing 6.3 in my package.json didn't help, because of the way babel modules install other babel modules underneath themselves, which have loose package specs (e.g. babel-core@6.3.26 will happily pull babel-generator@6.4.3 underneath itself).

Some kind of hotfix (error throw disable instruction or how to globally freeze at 6.3) would be helpful. I can look into it but if somebody knows off hand, I'm interested.

Fwiw it's complaining in JSX render() in my .js files which e.g. end with a but I don't know if it would happen with non-JSX stuff. My guess is that JSX gets transpiled to JS first though.

monfera commented Jan 14, 2016

Suddenly mandating the semicolons without a quick 'whoa, hang on a sec' option is throwing some obstacles. It was a surprise after npm install as I need to deploy code under time constraint, i.e. no time for MSI, and even fixing 6.3 in my package.json didn't help, because of the way babel modules install other babel modules underneath themselves, which have loose package specs (e.g. babel-core@6.3.26 will happily pull babel-generator@6.4.3 underneath itself).

Some kind of hotfix (error throw disable instruction or how to globally freeze at 6.3) would be helpful. I can look into it but if somebody knows off hand, I'm interested.

Fwiw it's complaining in JSX render() in my .js files which e.g. end with a but I don't know if it would happen with non-JSX stuff. My guess is that JSX gets transpiled to JS first though.

@CookPete

This comment has been minimized.

Show comment
Hide comment
@CookPete

CookPete Jan 14, 2016

even fixing 6.3 in my package.json didn't help, because of the way babel modules install other babel modules underneath themselves, which have loose package specs (e.g. babel-core@6.3.26 will happily pull babel-generator@6.4.3 underneath itself).

@monfera see my comment at theogravity/react-styleguide-generator-alt#9 (comment) with a suggested fix. In short, I used a partial shrinkwrap to pin babylon and it's dependents to 6.3.x, ie CookPete/react-player@e9bcd72

CookPete commented Jan 14, 2016

even fixing 6.3 in my package.json didn't help, because of the way babel modules install other babel modules underneath themselves, which have loose package specs (e.g. babel-core@6.3.26 will happily pull babel-generator@6.4.3 underneath itself).

@monfera see my comment at theogravity/react-styleguide-generator-alt#9 (comment) with a suggested fix. In short, I used a partial shrinkwrap to pin babylon and it's dependents to 6.3.x, ie CookPete/react-player@e9bcd72

@monfera

This comment has been minimized.

Show comment
Hide comment
@monfera

monfera Jan 14, 2016

@CookPete Thanks a lot Pete! I've reached a similar effect after seeing a terse fix suggestion, these were my steps: babel/babel#3225 (comment)

monfera commented Jan 14, 2016

@CookPete Thanks a lot Pete! I've reached a similar effect after seeing a terse fix suggestion, these were my steps: babel/babel#3225 (comment)

@GantMan

This comment has been minimized.

Show comment
Hide comment
@GantMan

GantMan Jan 14, 2016

@monfera - also see my note, that moving the code can make it semicolon free. Rather than managing each person's babel, I've just changed our standard.

GantMan commented Jan 14, 2016

@monfera - also see my note, that moving the code can make it semicolon free. Rather than managing each person's babel, I've just changed our standard.

@monfera

This comment has been minimized.

Show comment
Hide comment
@monfera

monfera Jan 14, 2016

@GantMan Nice! I'm not a semicolon-hater, just wanted to avoid changing the code base b/c of this. Babel might become more compliant but I think it's at least a big oversight in the spec if semicolons are required in the classes, because before that, we had (for better or worse) clarity about ASI and optionality, while now it feels really incoherent, mandating it sometimes, even in the absence of ambiguity. I guess it's an accidental oversight or someone who doesn't like Standard Style snuck it in :-)

monfera commented Jan 14, 2016

@GantMan Nice! I'm not a semicolon-hater, just wanted to avoid changing the code base b/c of this. Babel might become more compliant but I think it's at least a big oversight in the spec if semicolons are required in the classes, because before that, we had (for better or worse) clarity about ASI and optionality, while now it feels really incoherent, mandating it sometimes, even in the absence of ambiguity. I guess it's an accidental oversight or someone who doesn't like Standard Style snuck it in :-)

@monfera

This comment has been minimized.

Show comment
Hide comment
@monfera

monfera Jan 14, 2016

@GantMan ... just noticing this is the Standard Style board, so in that case I'm compelled to say that not using semicolons in ES2015 code is even more coherent (visually and denotationally, c.f. fat arrows, destructuring bind...) than with ES5. These and lots of other ES2015 changes (import/export, argument spread...) move JS toward less noise, so semicolons look really out of place, especially if someone favors functional composition, avoiding lots of broilerplate or other DRY violation.

monfera commented Jan 14, 2016

@GantMan ... just noticing this is the Standard Style board, so in that case I'm compelled to say that not using semicolons in ES2015 code is even more coherent (visually and denotationally, c.f. fat arrows, destructuring bind...) than with ES5. These and lots of other ES2015 changes (import/export, argument spread...) move JS toward less noise, so semicolons look really out of place, especially if someone favors functional composition, avoiding lots of broilerplate or other DRY violation.

@flying-sheep

This comment has been minimized.

Show comment
Hide comment
@flying-sheep

flying-sheep Jan 14, 2016

@GantMan unfortunately going back to class field assignment after the class declaration isn’t declarative.

i’d like to stick to my coding style and gain delarativeness instead of deciding between one or the other.

i hope people quickly return to ASI as outlined in tc39/proposal-class-public-fields#26 (comment)

flying-sheep commented Jan 14, 2016

@GantMan unfortunately going back to class field assignment after the class declaration isn’t declarative.

i’d like to stick to my coding style and gain delarativeness instead of deciding between one or the other.

i hope people quickly return to ASI as outlined in tc39/proposal-class-public-fields#26 (comment)

alyssaq added a commit to alyssaq/react-redux-table-example that referenced this issue Jan 22, 2016

rmehner referenced this issue in eHealthAfrica/grunt-tx Jan 31, 2016

@grigio grigio referenced this issue Feb 2, 2016

Closed

Standard JS #707

@flying-sheep

This comment has been minimized.

Show comment
Hide comment

flying-sheep commented Feb 2, 2016

@feross

This comment has been minimized.

Show comment
Hide comment
@feross

feross Feb 4, 2016

Member

Looks like I missed most of this discussion. Excellent to hear that the change will be reverted, @flying-sheep. Crisis averted.

Member

feross commented Feb 4, 2016

Looks like I missed most of this discussion. Excellent to hear that the change will be reverted, @flying-sheep. Crisis averted.

@feross feross closed this Feb 4, 2016

@nabn

This comment has been minimized.

Show comment
Hide comment
@nabn

nabn Mar 6, 2016

With babel 6, it looks like you don't need semi colons in class functions anymore. I updated standardjs, but it still gives me a warning.
What's the update on this?

nabn commented Mar 6, 2016

With babel 6, it looks like you don't need semi colons in class functions anymore. I updated standardjs, but it still gives me a warning.
What's the update on this?

@aakashsigdel

This comment has been minimized.

Show comment
Hide comment
@aakashsigdel

aakashsigdel Mar 31, 2016

+1 for support for arrow functions inside class as now babel compiles without the semicolon

aakashsigdel commented Mar 31, 2016

+1 for support for arrow functions inside class as now babel compiles without the semicolon

@wdoug wdoug referenced this issue Apr 24, 2016

Merged

Merged, working #39

@vslio

This comment has been minimized.

Show comment
Hide comment
@vslio

vslio May 13, 2016

I would also love to see support for this.

handleSearchBoxChange = (search) => {
  this.props.dispatch(FollowingActions.getFollowingSearch(search))
}

Using the above arrow function inside a class yields the following error...:
Parsing error: Unexpected token = (null)

... and the rest of the file doesn't get checked at all, even though there are style issues I purposely introduced. The moment I comment out the arrow function, the errors are highlighted immediately on the editor.

vslio commented May 13, 2016

I would also love to see support for this.

handleSearchBoxChange = (search) => {
  this.props.dispatch(FollowingActions.getFollowingSearch(search))
}

Using the above arrow function inside a class yields the following error...:
Parsing error: Unexpected token = (null)

... and the rest of the file doesn't get checked at all, even though there are style issues I purposely introduced. The moment I comment out the arrow function, the errors are highlighted immediately on the editor.

@vslio

This comment has been minimized.

Show comment
Hide comment
@vslio

vslio May 13, 2016

@jprichardson well, that was embarrassing... 😭
Thanks a lot for the quick reply–works like a charm! 👍

vslio commented May 13, 2016

@jprichardson well, that was embarrassing... 😭
Thanks a lot for the quick reply–works like a charm! 👍

tihonove pushed a commit to skbkontur/retail-ui that referenced this issue Oct 18, 2016

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.