This repository has been archived by the owner. It is now read-only.

Don't parse class properties without initializers when classProperties plugin is disabled, and Flow is enabled #300

Merged
merged 1 commit into from Jan 23, 2017

Conversation

Projects
None yet
4 participants
@DrewML
Copy link
Member

DrewML commented Jan 15, 2017

Q A
Bug fix? yes
Breaking change? yes (technically)
New feature? no
Deprecations? yes
Spec compliancy? no
Tests added/pass? yes
Fixed tickets
License MIT

On master, we'll currently parse the following, even with classProperties disabled

class Foo {
    bar;
}

This was happening because we defer the check for the plugin until we're inside the body of pp.parseClassProperty, so that Flow can parse the following:

class Foo {
    bar: string;
}

Once inside of pp.parseClassProperty, we were only checking for the plugin if an initializer had been provided.

We unfortunately can't just disallow Flow annotations for class properties when when classProperties are disabled, because Flow allows you to do:

class Foo {
   bar: string;

   constructor() {
       this.bar = 'bar';
   }
}

I marked this as a breaking change, but it's also technically a bug, so unsure if we want to defer to Babylon 7 or not.

@DrewML DrewML requested review from loganfsmyth and danez Jan 15, 2017

@DrewML DrewML changed the title Don't parse class properties without initializers when classPropertie… Don't parse class properties without initializers when classProperties plugin is disabled, and Flow is enabled Jan 15, 2017

@@ -771,8 +771,13 @@ pp.parseClassBody = function (node) {
};

pp.parseClassProperty = function (node) {
const noPluginMsg = "You can only use Class Properties when the 'classProperties' plugin is enabled.";
if (!node.typeAnnotation && !this.hasPlugin("classProperties")) {

This comment has been minimized.

@DrewML

DrewML Jan 15, 2017

Author Member

I'm not super happy about a flow-specific check leaking into the general parser, but I didn't see a straight-forward way to do this otherwise. Only other way I could see was to just have the flow extension for parseClassProperty invoke this with an optional second param of hadType, but even then, Flow-related stuff still leaks in.

This comment has been minimized.

@danez

danez Jan 16, 2017

Member

This code will not stay forever anyway, as as soon as class-properties are final, we can remove all this.

This comment has been minimized.

@hzoo

hzoo Jan 16, 2017

Member

We really need to do #178

This comment has been minimized.

@DrewML

DrewML Jan 16, 2017

Author Member

This code will not stay forever anyway, as as soon as class-properties are final, we can remove all this.

Good point

if (this.match(tt.eq)) {
if (!this.hasPlugin("classProperties")) this.unexpected();
if (!this.hasPlugin("classProperties")) this.raise(this.state.start, noPluginMsg);

This comment has been minimized.

@DrewML

DrewML Jan 15, 2017

Author Member

Pretty sure it's safe to provide a better message here. We should only get to this point if we're inside the top level of a class body and we've found either a = or linebreak after an identifier.

if (this.match(tt.eq)) {
if (!this.hasPlugin("classProperties")) this.unexpected();
if (!this.hasPlugin("classProperties")) this.raise(this.state.start, noPluginMsg);

This comment has been minimized.

@DrewML

DrewML Jan 15, 2017

Author Member

Should note I'm using this.state.start rather than node.start here, so we're pointing out the error where the = is, rather than what could be the start of a type annotation in the scenario that the code being parsed is:

class Foo {
   bar: string = 'bizz';
}

This comment has been minimized.

@danez

danez Jan 16, 2017

Member

Yes this was the same before, as this.unexpected() uses this.state.start

@DrewML DrewML added the i: bug label Jan 15, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 15, 2017

Current coverage is 97.30% (diff: 100%)

Merging #300 into master will decrease coverage by 0.16%

@@             master       #300   diff @@
==========================================
  Files            21         21          
  Lines          3993       4003    +10   
  Methods         478        480     +2   
  Messages          0          0          
  Branches       1178       1179     +1   
==========================================
+ Hits           3892       3895     +3   
- Misses           44         49     +5   
- Partials         57         59     +2   

Powered by Codecov. Last update 68a173c...0a1d649

@danez

danez approved these changes Jan 16, 2017

Copy link
Member

danez left a comment

lgtm

This could be potentially breaking, for people relying on this wrong behaviour. Although I'm not sure if that'S a real case, as babel would not transform class properties when they are not active, but flow is and you use class properties in your code. I think babel throws in that case if I remember right.
Should we do it in 7? Thoughts?

@@ -771,8 +771,13 @@ pp.parseClassBody = function (node) {
};

pp.parseClassProperty = function (node) {
const noPluginMsg = "You can only use Class Properties when the 'classProperties' plugin is enabled.";
if (!node.typeAnnotation && !this.hasPlugin("classProperties")) {

This comment has been minimized.

@danez

danez Jan 16, 2017

Member

This code will not stay forever anyway, as as soon as class-properties are final, we can remove all this.

if (this.match(tt.eq)) {
if (!this.hasPlugin("classProperties")) this.unexpected();
if (!this.hasPlugin("classProperties")) this.raise(this.state.start, noPluginMsg);

This comment has been minimized.

@danez

danez Jan 16, 2017

Member

Yes this was the same before, as this.unexpected() uses this.state.start

@DrewML

This comment has been minimized.

Copy link
Member Author

DrewML commented Jan 16, 2017

This could be potentially breaking, for people relying on this wrong behaviour. Although I'm not sure if that'S a real case, as babel would not transform class properties when they are not active, but flow is and you use class properties in your code

@danez looks like this wouldn't affect most Babel users.

This seems like it'd only break things consuming the parser directly (like babel-eslint), but I can't think of a way it could actually break code being built.

@danez danez merged commit 9929baa into babel:master Jan 23, 2017

3 checks passed

codecov/patch 100% of diff hit (target 97.47%)
Details
codecov/project Absolute coverage decreased by -0.16% but relative coverage increased by +2.52% compared to 68a173c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

danez pushed a commit that referenced this pull request Mar 1, 2017

@danez

This comment has been minimized.

Copy link
Member

danez commented Mar 1, 2017

I landed this in 7.0 now.

@DrewML

This comment has been minimized.

Copy link
Member Author

DrewML commented Mar 1, 2017

Thanks @danez!

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