Skip to content

Add missing ExecutionArgs export#1027

Merged
wincent merged 3 commits intographql:masterfrom
IvanGoncharov:excuteArgs
Sep 8, 2017
Merged

Add missing ExecutionArgs export#1027
wincent merged 3 commits intographql:masterfrom
IvanGoncharov:excuteArgs

Conversation

@IvanGoncharov
Copy link
Copy Markdown
Member

In #988 I forget to export ExecutionArgs in all necessary places 🤦‍♂️
This one-line PR fix that.
@wincent Can you please review it?

@IvanGoncharov
Copy link
Copy Markdown
Member Author

IvanGoncharov commented Sep 7, 2017

@wincent I was wondering why Flow silently ignored missing export. So I discovered that a few files missing @flow directive.
And lexer.js had /* @flow / since the first commit, not sure how Flow is parsing such comments.
I also unify @flow directives to always be in a first line, so you can use simple shell script to find all files without it.
After enabling flow checks on more files two new issue appeared which I fixed in the separate commit.

@wincent
Copy link
Copy Markdown
Contributor

wincent commented Sep 8, 2017

Thanks @IvanGoncharov.

FWIW I don't really like the hoisting up of the @flow annotation to the top; it smells of special-casing to me and @flow shouldn't be more special than any other annotation that we might use (and we do use quite a few at FB). Having said that, consistency is good, and there is a precedent for this style, so... ¯\(ツ)

@wincent wincent merged commit eb01a23 into graphql:master Sep 8, 2017
@IvanGoncharov IvanGoncharov deleted the excuteArgs branch December 14, 2017 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants