Babel v6.4 Requires semicolons after class properties #372

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

Projects

None yet
@cableray
cableray commented Jan 6, 2016

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

@dcousens
Collaborator
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
Collaborator
bcomnes commented Jan 7, 2016

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

@LinusU
Collaborator
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

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
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 CookPete added a commit to CookPete/react-player that referenced this issue Jan 7, 2016
@CookPete CookPete Use shrinkwrap to limit babel packages to 6.3
To circumvent the slightly lame new requirement for semicolons after class properties
feross/standard#372
630072c
@rmehner rmehner added a commit to eHealthAfrica/grunt-tx that referenced this issue Jan 7, 2016
@rmehner rmehner fix: stay with babel 6.3.x for now due to new semicolon rules
Closes #21.

See feross/standard#372 for more.
b7b1f5a
@cableray
cableray commented Jan 7, 2016

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

@GantMan
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
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..

@chen844033231 chen844033231 referenced this issue in chen844033231/react-workflow Jan 8, 2016
Closed

报错误:static propTypes = { }加逗号就可以了 #40

@torifat
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
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
Collaborator
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
Collaborator
LinusU commented Jan 8, 2016

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

@theogravity theogravity referenced this issue in theogravity/react-styleguide-generator-alt Jan 8, 2016
Closed

Babel 6.4 semicolon problem #9

@CookPete
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
Collaborator

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
Collaborator
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
Collaborator
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.

@DavidWells DavidWells referenced this issue in bananaoomarang/isomorphic-redux Jan 9, 2016
Closed

App may be broken with Babel ^6.4.0 #44

@dcousens
Collaborator
dcousens commented Jan 9, 2016 edited

@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

@torifat thanks for this #372 (comment)

@favorit
favorit commented Jan 12, 2016

@torifat thanks!

@nottombrown nottombrown added a commit to nottombrown/redux-auth-demo that referenced this issue Jan 13, 2016
@nottombrown nottombrown Add semicolons after class properties a la feross/standard#372 444b828
@nottombrown nottombrown referenced this issue in lynndylanhurley/redux-auth-demo Jan 13, 2016
Closed

Add semicolons after class properties #3

@monfera
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

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
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
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
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
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

@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 alyssaq added a commit to alyssaq/react-redux-table-example that referenced this issue Jan 22, 2016
@alyssaq alyssaq Moved static properties to after class declaration due to standard issue 51b9913
@feross
Owner
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
@chrome chrome referenced this issue in toptal/simple-react-calendar Feb 29, 2016
Merged

Prepare for release #27

@nabn
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

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

@wdoug wdoug referenced this issue in codefordenver/ketohero Apr 24, 2016
Merged

Merged, working #39

@vslio
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
vslio commented May 13, 2016

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

@tihonove tihonove pushed a commit to skbkontur/retail-ui that referenced this issue Oct 18, 2016
scalder requires semicolons after class stativ properties (feross/standard#372) 2af783c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment