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

[Flow] Arrow function type parameter declarations #54

Merged
merged 1 commit into from
Jun 24, 2016

Conversation

gabelevi
Copy link
Contributor

In Flow, you can declare type parameters for arrow functions. The syntax looks like this:

  const arrowFunctionWithoutTypeParams = (x) => x;
  const arrowFunctionWithTypeParams = <T>(x: T): T => x;

This is a little tricky to parse, especially with JSX. This PR attempts to solve the problem as follows

  1. Try to parse the expression as if it were JSX. JSX is probably more common and this will lead to less backtracking. If this fails, save the error. If the JSX plugin isn't in use, then skip this step.
  2. If the next token is <, then parse a type parameter declaration. Then parse an assignment expression. If that expression turns out to an ArrowFunctionExpression, then attach those type params to that expression. Otherwise, fail. On failure throw the JSX error, if it exists, or a new error.
  3. If this is not JSX And doesn't start with < then just parse an assignment expression as usual.

This was further complicated by the need to reinterpret tokens and mess with context. But at least it seems to work :)

@gabelevi
Copy link
Contributor Author

This fixes https://phabricator.babeljs.io/T7330

instance.extend("parseMaybeAssign", function (inner) {
return function (...args) {
let jsxError = null;
let state = this.state.clone();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't super obvious but cloning state is actually super expensive, for example there's a tokens array on there that gets cloned and this can sometimes number in the tens of thousands for really large files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh great, just noticed that state is only used within the JSX if, you can just move it into there. It should resolve the more pathological cases but the perf issue will still be there. We should really use babylon to use immutable data structures so this state cloning is really cheap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll move it in there. It will still be hit for every JSX expression, I'm afraid, but that's better than every expression :)

@sebmck
Copy link
Contributor

sebmck commented Jun 24, 2016

Thanks @gabelevi! I'll publish a new release now.

@hzoo
Copy link
Member

hzoo commented Jun 24, 2016

We aren't running the changes on babel in travis yet right?

@sebmck
Copy link
Contributor

sebmck commented Jun 24, 2016

@hzoo Nope.

@sebmck
Copy link
Contributor

sebmck commented Jun 24, 2016

I pushed a new release just now, I'll run make test-babel to ensure that I haven't broken anything...

@hzoo
Copy link
Member

hzoo commented Jun 24, 2016

Ok cool - it does suck that it would make the tests run a lot longer but worth doing. (installing it now locally too, seeing no issues)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants