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

Error on invalid Flow type annotation with default assignment #122

Merged
merged 1 commit into from Sep 15, 2016

Conversation

Projects
None yet
4 participants
@danharper
Copy link
Member

danharper commented Sep 11, 2016

In Flow, a type annotation must come before a default assignment.

// good
function x(foo: string = "1") {} 

// bad
function x(foo = "1": string) {} 

Babylon currently parses the right-side type annotation, attaching it to the AssignmentPattern.

I guess this would be a breaking change, though..

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Sep 12, 2016

Yeah I think that was confusing before. I think it's worth making it as a bug fix? @jeffmo

@danez

This comment has been minimized.

Copy link
Member

danez commented Sep 12, 2016

That is awesome thanks.

Regarding the breaking change, I think we had cases like this in the past where we made the parser more spec compliant which might break peoples workflow if they were relying on this wrong behaviour or had it by accident in their code. This PR is exactly the same case and imho we should make this a patch, as being compliant to the spec should be considered important to spread quickly, no matter if es, flow or jsx. Most of the time it is probably a bug anyway. We probably should add a note in the changelog about "more strict spec for ..." though

As flow is also failing on this, there are probably also not a lot of cases were people use this (if so it might be a bug in their code anyway) and people relying on this fact should be 0, at least I can't make up good reason for doing that.

And of course:
https://xkcd.com/1172/

@@ -1 +1 @@
function g(a:number=1, e=1:number) {}
function g(a:number=1, e:number=1) {}

This comment has been minimized.

@danez

danez Sep 12, 2016

Member

Weird that we were testing the wrong behaviour is working. 🤔

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 12, 2016

Current coverage is 94.04% (diff: 100%)

No coverage report found for master at 015035c.

Powered by Codecov. Last update 015035c...bb2b40b

@danharper

This comment has been minimized.

Copy link
Member Author

danharper commented Sep 12, 2016

👍 I agree it should be treated as a bug fix. Just thought I'd mention it, as I know in the past I've had at least one file which wasn't being type-checked with Flow, but had some type annotations.

@hzoo hzoo added Tag: Bug Fix and removed i: bug labels Sep 12, 2016

@danez

danez approved these changes Sep 15, 2016

@danez danez merged commit 64145b0 into babel:master Sep 15, 2016

3 checks passed

codecov/patch 100% of diff hit (target 94.04%)
Details
codecov/project Absolute coverage decreased by -2.72% but relative coverage increased by +3.22% compared to bc6aa62
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.