Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(injector): support arrow functions with no parenthesis #12890

Closed
wants to merge 4 commits into from
Closed

fix(injector): support arrow functions with no parenthesis #12890

wants to merge 4 commits into from

Conversation

shahata
Copy link
Contributor

@shahata shahata commented Sep 18, 2015

No description provided.

@shahata
Copy link
Contributor Author

shahata commented Sep 18, 2015

I initially upgraded jscs so it will parse es6 syntax correctly, but since eventually I used eval in order to not break old browsers, it is not really needed. I think we can upgrade jscs anyway, so I'm leaving this commit as is.

@@ -62,17 +62,23 @@
* Implicit module which gets automatically added to each {@link auto.$injector $injector}.
*/

var ARROW_ARG = /^([^\(]+)=>/;
Copy link
Member

Choose a reason for hiding this comment

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

This RegExp would incorrectly match arg1 => { var arrow = '=>'; } (it would match greedily until the last => before the first ().
The + should be made non-greedy: /^([^(]+?)=>/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@gkalpak
Copy link
Member

gkalpak commented Sep 19, 2015

I left a couple of comments.
LGTM other than that.

@shahata
Copy link
Contributor Author

shahata commented Sep 20, 2015

Why isn't this running in travis? :(

@gkalpak
Copy link
Member

gkalpak commented Sep 20, 2015

It did run initially I think (https://travis-ci.org/angular/angular.js/builds/81241841).
I've seen it too in other PRs. I might happen when you force-push an amended commit.

@gkalpak
Copy link
Member

gkalpak commented Sep 20, 2015

Did someone restartd it or did it start on its own ?

@shahata
Copy link
Contributor Author

shahata commented Sep 20, 2015

I created another pull request on a cloned branch in hopes of somehow triggering travis and it seems to have worked :)

@shahata
Copy link
Contributor Author

shahata commented Sep 20, 2015

Okay, Travis finally approves. I'll merge this tomorrow morning unless anyone else has more comments

@lgalfaso
Copy link
Contributor

@shahata can you please squash c2e0ecf 427db62 into one commit. It is expected for some refactor to happen as part of a fix. The other commits are fine as is

Shahar Talmi added 4 commits September 21, 2015 08:11
with arrow functions parenthesis are optional in case you have exactly
one argument to the function. the previous regexp assumed function
arguments are always inside parenthesis and so it didn't annotate
functions like `$http => $http.get(...)` correctly
@shahata shahata closed this in 03726f7 Sep 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants