Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

export flow types in build (without test folders) #412

Merged
merged 4 commits into from
Nov 1, 2016
Merged

export flow types in build (without test folders) #412

merged 4 commits into from
Nov 1, 2016

Conversation

nodkz
Copy link
Contributor

@nodkz nodkz commented Jun 24, 2016

Another implementation of @nmn PR

Flow types very needed to avoid this trash: https://github.com/nodkz/graphql-compose/blob/master/src/definition.js (and graphql-js as git submodule)
Also solved @nmn issue #400

FYI npm run cp-flow correctly works if sub-folders exist in dist/ otherwise it prints No such file or directory.

Another implementation of @nmn PR

Flow types very needed to avoid this trash: https://github.com/nodkz/graphql-compose/blob/master/src/definition.js (and graphql-js as git submodule) 
Also solved @nmn issue #400

FYI `npm run cp-flow` correctly works if sub-folders exist in `dist/` otherwise it prints `No such file or directory`.
@ghost ghost added the CLA Signed label Jun 24, 2016
@nmn
Copy link

nmn commented Jun 25, 2016

sad that this had to be duplicated!

I can close my PR, or add this to my PR. whatever is preferred by the GraphQL team.

@nodkz
Copy link
Contributor Author

nodkz commented Jun 25, 2016

@nmn if you want save authority and think that my changes is better, please check it and change your PR. After that drop me line, and I close my PR. Cause without your solution, my PR was not be created.

On my mac you command throw me duplicated lines and files from _tests. So easiest way to fix it for me, was edit package via github interface. And of course one more time bump @leebyron to work with PRs.

@nmn don't be sad, you are awesome!

@leebyron
Copy link
Contributor

Thank you both for your work on this, @nodkz and @nmn! Sorry for my delay in reviewing these (I've been taking some personal time off recently). I'm really excited to get flow types into the release and am thankful for your research in the best way to do so.

@nodkz
Copy link
Contributor Author

nodkz commented Jun 30, 2016

@leebyron this is not better solution, because following files:
buildASTSchema.js
extendSchema.js
... produce more than 80 errors in my lib. So may be somebody will have same problems.

Right now I'm using graphql-js as submodule, and remove '@flow' line from above 2 files and have zero errors.

So how would be better totally exclude utilities folder from flow, or just this two files? Orsomething other ...

@leebyron
Copy link
Contributor

Can you explain more about the errors you're seeing? Are they real issues in your usage or are they issues with the type signatures of those files?

@nodkz
Copy link
Contributor Author

nodkz commented Jun 30, 2016

There are some problems with map functions under flow 0.27.

I'll give detailed information tomorrow. Out of work right now.

@leebyron
Copy link
Contributor

Ok - I'll delay on a merge until you report back

@nmn
Copy link

nmn commented Jun 30, 2016

@nodkz @leebyron I tend to think that if there are type errors, its because you're doing things that don't match the types defined in the library itself.

I feel like this is the best way to expose flow types in most cases as you get the actual flow types used by the library authors and not something tacked on after the fact.

Perhaps some flow types are too specific and cause too many errors, but that means that we should try and fix that in the source of the library itself and not by tacking on inaccurate type definitions.

P.S. closing my other PR now.

@nmn nmn mentioned this pull request Jun 30, 2016
@nodkz
Copy link
Contributor Author

nodkz commented Jul 1, 2016

Just clone fresh master of graphql-js.
On flow 0.27 produce following 77 errors: https://gist.github.com/nodkz/486a92285ec7ddebe9823508fb138a12

For fast view:
screen shot 2016-07-01 at 11 51 03

So this PR should be delayed until fixing flow errors on 0.27.
I have no time to find root of errors and make PR, cause I have 2 weeks gap on my project.

@nodkz
Copy link
Contributor Author

nodkz commented Jul 1, 2016

@leebyron I think, it will be cool and realized super fast, if you ask guys from flow to realize a call stack for error, and after that ask guys from nuclide to implement this feature in the atom-nuclide. Cause work with flow cli is very unproductive. And fixing such errors like above cease be a kinda guessing.

@iamchenxin
Copy link
Contributor

iamchenxin commented Jul 1, 2016

@nodkz @nmn @leebyron Those errors in Flow 0.27 are some errors were ignored in 0.26.
77 errors are really huge,some of them maybe need rewrite some code logic, or otherwise do some duplicated checks to make flow passed. (Im not sure which way is prefer. Is there some code style guide for this ?)
ex: produceTypeDef() | buildASTSchema.js#L271

function produceTypeDef(typeAST: Type): GraphQLType {
    const typeName = getNamedTypeAST(typeAST).name.value;
    const typeDef = typeDefNamed(typeName);
    return buildWrappedType(typeDef, typeAST);
}

Basically produceTypeDef get some dynamic runtime Type from typeDefNamed so it returned a full-range type GraphQLType. But in downstream,the GraphQLType was static split to subsets (GraphQLInputType or GraphQLOutputType). Not sure why 0.26 let those static split pass.
To resolve this,either rewrite the code logic to reflect runtime-check to static-check or do a simple check at every line using produceTypeDef (like buildASTSchema.js#L331 ).

It would be great if there are some inner plan to fix those Flow errors in Facebook,or i can plan to fix them,cause i find some errors in my package(used flow typed graphql) with Flow 0.27 too. 0.27 is great, i should stay in 0.27,should not temporarily rollback to 0.26.
PS: I have never speak English with someone in real life (off-line),so forgive my poor English.(If there are some misunderstanding in my words).

@iamchenxin
Copy link
Contributor

iamchenxin commented Jul 2, 2016

fix all errors with flow 0.27 in #420 ,will test them tomorrow( and write unit test).

@nodkz
Copy link
Contributor Author

nodkz commented Jul 6, 2016

@iamchenxin awesome work! Thanks for help.

@leebyron leebyron closed this in c7700f2 Jul 6, 2016
@leebyron leebyron reopened this Jul 6, 2016
@nodkz
Copy link
Contributor Author

nodkz commented Jul 7, 2016

After your last changes and updating to flow 0.28 all became perfect!
Tested with own modules. I'm also migrated to such kind of flow-types export: https://github.com/nodkz/graphql-compose-relay/blob/master/package.json#L59

Found a little bit clear bash-solution for finding list of js files, without __tests__ and __mocks__ folders 9f8b1cb.
PR may be merged.

Example of how dist folder filled with .flow files:
screen shot 2016-07-07 at 21 35 51

@nmn
Copy link

nmn commented Jul 8, 2016

@leebyron Your Move!!

@iamchenxin
Copy link
Contributor

iamchenxin commented Jul 8, 2016

There is no type export in src/index.js at current master, will add them later?

@ghost ghost added the CLA Signed label Jul 12, 2016
@leebyron leebyron merged commit 11827c5 into graphql:master Nov 1, 2016
nodkz added a commit to nodkz/emitter that referenced this pull request May 29, 2017
…annotations

Describe classes via `flow-typed` is too lazy and it may stale with time. 
The moreover types are already in the code. So let export them, like it done in `graphql-js`.
Related: graphql/graphql-js#412
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.

None yet

4 participants