Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Support defaults in Flow's type parameter declarations #25

Merged
merged 1 commit into from
May 17, 2016

Conversation

gabelevi
Copy link
Contributor

The primary goal of this commit is to add the ability to parse type parameter
declarations with defaults, like type Foo<T = string> = T. While I was in the
code, I fixed a few small things, like

  • Type parameter declarations need 1 or more type parameters.
  • The existential type * is not a valid type parameter.
  • The existential type * is a primary type
  • The param list for type parameter declarations now consists of
    TypeParameter nodes

The primary goal of this commit is to add the ability to parse type parameter
declarations with defaults, like `type Foo<T = string> = T`. While I was in the
code, I fixed a few small things, like

* Type parameter declarations need 1 or more type parameters.
* The existential type `*` is not a valid type parameter.
* The existential type `*` is a primary type
* The param list for type parameter declarations now consists of
  `TypeParameter` nodes
@gabelevi
Copy link
Contributor Author

@kittens or @hzoo - Is there anything I can do to help get this PR merged?

@sebmck sebmck merged commit 1c48c4c into babel:master May 17, 2016
@sebmck
Copy link
Contributor

sebmck commented May 17, 2016

@gabelevi Sorry for the delay. Were there any new node types/properties introduced that we necessitate changes to the code generator and traversal definitions in Babel?

@gabelevi
Copy link
Contributor Author

@kittens - sorry, I'm not quite understanding what you're asking. Are you asking about the downstream effects of this PR and what will have to change? I don't think I know enough about the code generator and traversal definitions to comment. Naively, a transform to strip away flow types shouldn't be affected, since this is a change within the type space. However a general visitor might notice that the list of type params is now an array of TypeParameter rather than an array of Identifier

@glenjamin
Copy link

This PR appears to fix https://phabricator.babeljs.io/T7365 as well. I had started to have a go at this but only got as far as expanding a test case to cover more existential scenarios before discovering it already worked on master.

It might be worth including these examples to cover the newly expanding existential support? (I added the flow header so that I could run flow on the testcase to check it agreed with this parser)

diff --git a/test/fixtures/flow/type-annotations/existential-type-param/actual.js b/test/fixtures/flow/type-annotations/existential-type-param/actual.js
index 83f6d6f..a10100a 100644
--- a/test/fixtures/flow/type-annotations/existential-type-param/actual.js
+++ b/test/fixtures/flow/type-annotations/existential-type-param/actual.js
@@ -1 +1,8 @@
-type Maybe<T> = _Maybe<T, *>;
+/* @flow */
+type Maybe<T> = Map<T, *>;
+class ABC extends Array<*> {}
+function abc(a: *): number {
+  return 1;
+}
+var obj: {a: *, b: *} = {a: 1, b: "a"};
+var x: * = 17;

@gabelevi gabelevi deleted the default branch June 6, 2016 15:29
@hzoo
Copy link
Member

hzoo commented Jun 8, 2016

I think this is causing an issue in some of the babel tests babel/babel#3519 (a readme change). I guess one of the benefits with keeping it in the same repo was knowing it a change in babylon affected babel without having to guess? We should probably do something like https://github.com/babel/babylon/pull/21/files or automatically run a test with new babylon changes?

@hzoo
Copy link
Member

hzoo commented Jun 8, 2016

WIP PR babel/babel#3520 - and yeah actually these are technically a breaking change (at this specifically the one for type params is now an array of TypeParameter rather than an array of Identifier @gabelevi

The param list for type parameter declarations now consists of TypeParameter nodes

So I'm not sure what we should do here - revert ^ as a patch? If we call that a breaking change then its a breaking change in babel as well? Or given this was only introduced in March and not the most common feature there shouldn't be anyone relying on this

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants