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

fix all errors with flow 0.28 #420

Closed
wants to merge 2 commits into from
Closed

Conversation

iamchenxin
Copy link
Contributor

@iamchenxin iamchenxin commented Jul 2, 2016

Fix all errors(update Flow to 0.27), will write unit test later.(flow 0.27 errors motioned in #412 )
And keep GraphQLType to a normal type (not update it to a recursive type):

export type GraphQLType =
  GraphQLNamedType|
  GraphQLList |
  GraphQLNonNull;

cause in flow 0.27 GraphQLList<GraphQLType> was still resolved to GraphQLList<any>,seems its no difference between GraphQLList and GraphQLList<GraphQLType>. I am asking this behaviors in flow/issues/2024, will test this tomorrow, the any type should be cleaned.

@iamchenxin
Copy link
Contributor Author

iamchenxin commented Jul 2, 2016

Confirmed its a bug in Nuclide which recursive types may be displayed as Type<any>.(just a wrong type hint). Flow 0.27 work well with recursive types.
ex:

type GraphQLType =
  GraphQLNamedType|
  GraphQLList<GraphQLType> |
  GraphQLNonNull<
    GraphQLNamedType|
    GraphQLList<GraphQLType>
  >;

Nuclide will display GraphQLList<GraphQLType> as GraphQLList<any>.
But In truth, Flow 0.27 work correctly, will resolve GraphQLList<GraphQLType> as GraphQLList<GraphQLType>.

So it is suitable to update GraphQLType to a recursive type.(to completely express what it is)
Will do this later,maybe should use another Pull Request to update GraphQLType to a recursive type.
This PR should keep in fixing exsiting Flow Type Error.

@iamchenxin iamchenxin changed the title fix all errors with flow 0.27,will write unit test later fix all errors with flow 0.27 Jul 2, 2016
@leebyron
Copy link
Contributor

leebyron commented Jul 6, 2016

Thanks so much for investigating this! Ideally we introduce as little runtime code as possible when fixing flow errors - I'm not sure this warrants the new utility functions you've added. Lets attempt to solve the type errors just with better types first.

@nodkz
Copy link
Contributor

nodkz commented Jul 6, 2016

Flow 0.28 landed
https://flowtype.org/blog/2016/07/01/New-Unions-Intersections.html

@iamchenxin may be you make another try with 0.28 and it helps find simpler solution.

@iamchenxin
Copy link
Contributor Author

iamchenxin commented Jul 6, 2016

@leebyron Sure, Static Type is always best, Let me take a look at new master. and will add a process.env.NODE_ENV switch for all of those dynamic test(if Temporarily using them). So they will just run in development environment , have no produce time-load.

@iamchenxin
Copy link
Contributor Author

iamchenxin commented Jul 6, 2016

@nodkz let me take a look at 0.28. There are some inner API in facebook,like React using some of them. Maybe some of those inner API will have some magic effects to get things work.

@iamchenxin iamchenxin changed the title fix all errors with flow 0.27 fix all errors with flow 0.28 Jul 6, 2016
@iamchenxin
Copy link
Contributor Author

iamchenxin commented Jul 6, 2016

@leebyron Update this PR with new master.Marked all Type conflicts with checks. Some of them are things should be check.

Maybe should consider to restrict using functions like isInputType . Those functions are pure runtime check(accept values but return a boolean,that will lead to Flow lose Type information.)

Keep the GraphQLType still use any ,should update it to * later.

export type GraphQLType =
  GraphQLNamedType |
  GraphQLList<any> |
  GraphQLNonNull<any>;

if change 'GraphQLList' to GraphQLList<*> will get additional 33 errors. I am not sure which code style are prefer to fix those errors.
For example: buildASTSchema.js#286: in there -> The Type get from produceType will finally need a check by code to get a certain type,can not do this by Flow, cause produceType produced Types from string value.(It produce Type in runtime.) Although logically, we can infer that if we input something it will return a certain data,but its not something about static,things like that should be dynamic.( I give this a little trick in typecast.js.If prefix dev in runtime-check functions(dev.castToInputType), when process.env.NODE_ENV !== 'dev' they will just do nothing but return the passed in value)
Some of runtime checks is not necessary to avoid.(cause with Flow,we only need to check once,Dynamic to Static,and will not to do duplicate check with this rules ). For example its not necessary to split buildFieldType to buildInputFieldType and buildOutputFieldType in extendSchema.js#521. Cause they are using a same code logic indeed, if some runtime data produced ,there should have some check(here or there,but only need once) to check logic errors or something happed when code are ran out of the Ideal environment.( the test code maybe is unit test or dynamic check or something else). So split or not , a coding check should always need. ( Split is okay,but not Split is not bad).

@leebyron
Copy link
Contributor

leebyron commented Jul 6, 2016

Thanks again for looking into this. I think given the current state of things there should be an easier path to gaining flow 0.28 support, I'll investigate

leebyron added a commit that referenced this pull request Jul 6, 2016
flow check now passes for the existing v0.26 as well as v0.27 and v0.28, so upgrading to the latest version.

Fixes #412, #418, #420
@leebyron
Copy link
Contributor

leebyron commented Jul 6, 2016

Closing since the upgrade ended up being pretty straight forward after the other improvements to type checking were made elsewhere for v0.27

@leebyron leebyron closed this Jul 6, 2016
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