`babylon`: throw parse error if class properties do not have a semico…

[hzoo]

…lon (fixes T6873)

Fixes https://phabricator.babeljs.io/T6873. Ref https://github.com/jeffmo/es-class-fields-and-static-properties/issues/25

not using isLineTerminator since it tries to eat the semicolon

// TODO
pp.isLineTerminator = function () {
  return this.eat(tt.semi) || this.canInsertSemicolon();
};

[codecov-io]

Current coverage is 84.94%

Merging #3225 into master will decrease coverage by -0.22% as of 976edfc

@@            master   #3225   diff @@
======================================
  Files          215     215       
  Stmts        15653   15654     +1
  Branches      3353    3354     +1
  Methods          0       0       
======================================
- Hit          13331   13298    -33
- Partial        678     712    +34
  Missed        1644    1644       

Review entire Coverage Diff as of 976edfc

Powered by Codecov. Updated on successful CI builds.

[jamiebuilds]

This is an interesting one, it's kind of a breaking change, but also for spec compliance. How should we handle it?

[jmm]

@thejameskyle I think in general spec-compliance failures are bugs and correcting them should be versioned as such. People need to figure out correct syntax (e.g. by consulting the spec / articles [though those can have errors] / discussions) as opposed to having an expectation that Babel interpreting it a certain way validates it and that's a contract to continue interpreting it that way. In other words, if the syntax was broken in the first place I don't think Babel should be responsible for not "breaking" it by fixing its interpretation.

I think it's going to be problematic to have to bump the major version to do spec conformance fixes, especially if plugin and core component versions move in parallel.

And of course this syntax isn't even standardized yet so should be seen as experimental and unstable even if you do follow its "spec". Not entirely sure what versioning scheme makes sense for that.

[jamiebuilds]

Yeah but again this was supposed to be one of the benefits of splitting everything up into plugins, they could be versioned properly even through spec compliance chances.

[jmm]

@thejameskyle yeah I'm not arguing against that (from your recent comment I actually thought you came down in favor of maintaining the single version line), but what is the proper versioning (and is it different for standardized stuff vs. proposals)?

Is every spec compliance fix going to be considered a breaking change? Perhaps that actually makes more sense for proposals, but if incorrect interpretation of standardized stuff is corrected is that not a bug fix? I'm not being a troll, I just need to understand (and collectively perhaps we need to figure it out, if I'm not the only one who's uncertain).

What about https://github.com/babel/babel/pull/3220, is that a breaking change in Babylon? (To me it's a bug fix.)

On another note (not a spec-compliance issue), https://github.com/babel/babel/pull/3176 could break people's stuff if they were relying on the old behavior, but I interpreted it as a bug fix (and I think it would be insane to bump the major of everything -- babel-core etc. -- to change it).

[jamiebuilds]

Idk, if this were a spec compliance change in a plugin, I would favor bumping on any breaking change, but since its in the parser we'd have to bump everything (babel-core, etc), so idk. I guess this isn't that significant of a change.

[sebmck]

Semver is very inadequate for a compiler/parser. I'd say this is fixing a bug since we weren't following the specification.

[hzoo]

should of cc @jeffmo to be sure (obvious in hindsight)

[jeffmo]

Woo! Thanks for fixing this :)

[hzoo]

Can use jscs to autofix if necessary. Made a simple PR https://github.com/jscs-dev/node-jscs/pull/2057.

Instructions: https://gist.github.com/hzoo/cc8af2132d1775d8511d

[mgcrea]

Just stumbled accross this, why do we need to throw an error here while it is optional everywhere else? Does the spec is really that strict (should crash if no semicolon)? Shouldn't this be a linting issue?

[hzoo]

Yeah I thought it was a linting issue initially as well - in the OP https://phabricator.babeljs.io/T6873

[jmm]

@mgcrea Did you check out the reference link in the OP?

[mgcrea]

Nop, but I should have! I got it.

[dylanpyle]

I'll add my +1 for the "this broke our builds and took me by surprise". I do agree that a spec-compliance issue is more of a bugfix than anything else, but is there a better way to call these out in future? Maybe introducing a transition version with a loud (but non-breaking) deprecation warning?

[hzoo]

@dylanpyle Yeah we can definitely try making it easier to transition. Sorry about that, we will add a deprecation message for future breaking changes in the parser and figure that process out..

For this particular issue, you can use my jscs branch to autofix the semicolons if that's helpful which I mentioned above https://github.com/babel/babel/pull/3225#issuecomment-169676621.

[AndrewIngram]

FWIW, this hit me too, and all my dependencies are pinned to exact versions. Code that was working fine locally failed in staging, I was only able to reproduce locally by blowing away my node_modules and installing fresh. I'd have been able to avoid this if i'd been using shrinkwrap. However, I should add that my mistaken use of a semicolon here from mimicking widespread examples of using class properties.

[loganfsmyth]

@AndrewIngram Generally pinning specific versions isn't a safe approach for unexpected regressions. If you know there is a problem with a specific direct dependency then it makes sense, but it doesn't address the issue of unexpected changes like this where you were potentially relying on bugged behavior.

In general you should use broad meaningful semver ranges wherever possible in your package.json and use https://docs.npmjs.com/cli/shrinkwrap to lock your whole dependency graph if this is a concern, and do so only and the very top level of your application, not in libraries.

[AndrewIngram]

I did mention shrinkwrap in my comment :).

I'm fully aware of the problems in my approach, and in this particular case shrinkwrap is the only thing that would have prevented the issue. Unfortunately our servers are still on npm 2, where shrinkwrap... could be better.

It wasn't a huge issue, since it was caught in staging and I fixed it pretty quickly. So I'm not commenting to ask for help, merely to add myself as a data point of someone who was affected by this.

Edit: I should add, having had a quick look at libraries that are setup to use the 'no-extra-semi' eslint rule will now break, because eslint also uses the buggy interpretation class properties.

[evocateur]

@AndrewIngram I feel your npm2 + shrinkwrap pain. I hacked together a utility to make it suck less: https://www.npmjs.com/package/shrinkwarp

[loganfsmyth]

@AndrewIngram I apparently just can't read, my bad :P

[rstacruz]

According to this comment in the es-class-fields-and-static-properties proposal, this example below should be fine, and ASI should assume there's a semicolon after 'paused':

class Player {
  static status = 'paused'
  start () { }
}

however, babel 6.4 (as seen in babel-cli, babylon, babel-core, babel-preset-stage-0 and so on) will throw an error here:

SyntaxError: hi.js: A semicolon is required after a class property (2:26)
  1 | class Player {
> 2 |   static status = 'paused'

[jeffmo]

Just clarifying that right now in the proposal, semicolons are required always. I'm planning to bring this up at TC39 at the end of the month and discuss some of these concerns to see what we can come up with.

[bcomnes]

Thanks @jeffmo, is there any way to listen in on the discussion or track minutes? I was reading this requirement like @rstacruz where ASI covers this requirements other than the places where it doesn't already.

[hzoo]

@bcomnes There's https://github.com/rwaldron/tc39-notes if you didn't know about that already

[jmm]

Thanks @jeffmo! I think the people in the issue on your repo might be right that it just works like ASI in general though: a literal semicolon is not required where the ASI rules make it parse as valid constructs, and in a case like your example you need to disambiguate it with a literal semicolon or it ends up a syntax error.

[dtothefp]

would be nice if changes like this were warned prior to change, potentially using DEBUG=babel as I've seen in a couple places in the Babel repo. Another option would be to introduce warnings somehow into babel-eslint some time prior to release. There were big breaking changes in Babel 6 and would have been nice to have a heads up https://phabricator.babeljs.io/T2645, https://phabricator.babeljs.io/T2212

[michaelficarra]

@rstacruz That is a bug in babel, introduced by this PR. That semicolon does not need to be in the program text, as ASI will insert it when the parser sees the start token.

[johnnyji]

Hey guys,

I've written a codemod that fixes this issue, feel free to use it:

Here's the Gist: https://gist.github.com/johnnyji/72d10b10b4a4fab10f3f Here's the ASTExplorer: http://astexplorer.net/#/mdxtQg5vR8/7

In order to run this:

1. Make sure you temporarily downgrade your .babelrc and package.json to use Babel 5
2. npm i -g jscodeshift
3. Run from terminal: jscodeshift -t ./path/to/codemod/file ./path/to/folder/to/transform
4. Reupgrade to Babel 6

Cheers!

[ziemkowski]

So what is the resolution to this?

@kittens This commit should be reverted, no? Based on the discussion in this thread and others, it sounds like ASI should cover this the way it was working before, making this not an actual bug in the first place.

We've locked our Babel version below 6.3 in the meantime. If anybody else reading this thread needs to avoid this, updating your package.json like so will keep you going until this is resolved:

    "babel": "<6.3.0",
    "babel-cli": "<6.3.0",
    "babel-core": "<6.3.0",
    "babylon": "<6.3.0",

[monfera]

@ziemkowski Thanks for the suggestion of switching to "<6.3.0" but let me clarify the solution in case others run into it.

Changing these in the top-level dependencies / devDependencies in the package.json did NOT solve the issue, because npm install still merrily installed 6.4.* stuff underneath, as the specs typically constrain versions below 7.0.

However, making a shrinkwrap and modifying it worked out, with the following steps:

1. Ran npm shrinkwrap --dev which creates an npm-shrinkwrap.json file.
2. Searched for these four items, and those only, e.g. "babel-core". For each of the hits, replaced the version so that you have "version": "<6.3.0" for them.
3. Since npm-shrinkwrap.json also contains the "resolved" property, for each of these replacements, I deleted the line containing the "resolved" property.
4. Went into the node_modules directory and did rm -r * - not sure if it was necessary though, I just didn't want that shrinkwrapped install falls back on a preexisting version in case it encounters a package with a deleted "resolved" property.
5. npm install
6. At this point, the install went fine and the application started to work again.

Last, optional step: I ran npm shrinkwrap --dev in the expectation that it adds back proper "resolved" properties, which it thankfully did, but it also brought in additional dependencies - evidently, the shift in the versions caused shifts in what the dependencies depended on. Naturally, some of these newly appearing dependencies brought in some of the elements listed, e.g. "babel-traverse" 'started to' depend on "babylon", and it of course put in version 6.4.2 :-) However I didn't bother with these; rebuilt with these anyway and the application still worked.

[monfera]

@ziemkowski @CookPete has an analogous recipe: https://github.com/feross/standard/issues/372#issuecomment-171618575

[flying-sheep]

time to revert this! https://github.com/jeffmo/es-class-fields-and-static-properties/issues/26#issuecomment-177720642

[ziemkowski]

@kittens is this something you plan to take care of or should one of us submit a pull request backing this out? I'd just do it but that seems like poor etiquette.

[flying-sheep]

@ziemkowski this is the bug for it (title is worded a bit strangely).

[jmm]

@flying-sheep

I will follow up with Babel and to get this fixed so that ASI works as expected there.

https://github.com/jeffmo/es-class-fields-and-static-properties/issues/26#issuecomment-177720642

[hzoo]

We'll do a PR reverting the commits shortly (I made another commit after this fixing error location)

[ChrisCinelli]

@hzoo? Do you have an eta? End of this week? We need to move to Babel 6 for another feature but this one is holding us back.

[hzoo]

Not sure, I put what we want to get in https://github.com/babel/babel/milestones/6.5.x - maybe this weekend? https://github.com/babel/babel/pull/3332

[hzoo]

Ok! Released https://github.com/babel/babel/releases/tag/v6.5.2

[flying-sheep]

cool stuff!