Skip to content

Make consuming graphql-js easier with leveraged flow support #248

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

Closed
kastermester opened this issue Nov 22, 2015 · 16 comments
Closed

Make consuming graphql-js easier with leveraged flow support #248

kastermester opened this issue Nov 22, 2015 · 16 comments

Comments

@kastermester
Copy link

Hello,

Looking at the entire code base for graphql I see that it uses flow everywhere, which is quite nice :)

However when I am consuming the library, there's no way for me to leverage all of this type goodness. I have taken a stab at writing up a library definitions file, but that feels kinda icky to me as well, and besides I have quite a few issues getting them to work.

It would be really cool if there was a way to publish the package in a way where people using flow could use the types written inside the code base. I am aware of the project that tries to embed the types into comments, but I am not entirely sure how far along that project is in making it actually work with such a sizeable code base.

Are there any plans, or anyone with a better idea of how to export the flow types in this package to other users? An officially maintained library would of course work for me as well, but my guess would be that that could get out of hand to maintain quite fast.

@leebyron
Copy link
Contributor

leebyron commented Dec 1, 2015

Yes, this is something we want to do! There are still a few issues with flow's treatment of type definitions when deployed to npm, but once those are fixed we will include flow definitions with this package.

I'll keep this task open to track progress.

@kastermester
Copy link
Author

With the recent changes to flow 0.19, where it will treat files with .flow extension as more important than those without with regards to importing types, should it not be possible to do this in a "dumb" way, by simply leaving the original source files with the added extension?

@kastermester
Copy link
Author

Do you have any comments regarding my suggestion @leebyron? If you think it is the right approach I would be happy to provide a PR to do this.

@leebyron
Copy link
Contributor

I'm not sure actually, I'll consult the flow team on their desired best practices


Sent from Mailbox

On Sun, Dec 13, 2015 at 11:13 AM, Kaare Hoff Skovgaard
[email protected] wrote:

Do you have any comments regarding my suggestion @leebyron? If you think it is the right approach I would be happy to provide a PR to do this.

Reply to this email directly or view it on GitHub:
#248 (comment)

@loganlinn
Copy link

@leebyron Any updates on this? From what I can see, the .flow route seems to be the suggested solution facebook/flow#910 facebook/flow#593

@samwgoldman
Copy link
Contributor

Lee, you can always walk over to my desk if you have a question!

@iamchenxin
Copy link
Contributor

iamchenxin commented May 11, 2016

@samwgoldman Is facebook use .flow for grapgql internally now ?
I found something strange.
When i use babel-preset-fbjs to convert src/ to flow, there are some strange results. Like :
with config

fbjsConfigurePreset({
  autoImport: false,
  target: 'flow',
})

will get some result like :
src

export type GraphQLScalarTypeConfig<InternalType> = {
  name: string;
  description?: ?string;
  serialize: (value: mixed) => ?InternalType;
  parseValue?: (value: mixed) => ?InternalType;
  parseLiteral?: (valueAST: Value) => ?InternalType;
}

=> result

export type GraphQLScalarTypeConfig<InternalType> = {
  name: string;
  description?: ?string;
  serialize(value: mixed): ?InternalType;
  parseValue?(value: mixed): ?InternalType;           // wrong convert 
  parseLiteral?(valueAST: Value): ?InternalType;    // wrong convert 
};

According to fbjs/commit/7baf1f i should use the plugins included in preset-fbjs to avoid babel's bug. Did i do some incorrect config for fbjsConfigurePreset?

: Now im temporarily directly rename all src file to .js.flow . Seems temporarily work well for awhile.

@iamchenxin
Copy link
Contributor

@samwgoldman oh, i got some idea. 'graphql' did not use provides-module module-map, so it is bug free. Do not need babel to convert js to js.flow. Just need rename all src. Am i right?

@iamchenxin
Copy link
Contributor

iamchenxin commented Jun 6, 2016

@samwgoldman @kastermester I made a solution (flow-dynamic ) for this . See pr:89 i push to Graphql-relay(detail codes) to resolve flow type casting between Packages.
this pr:89 is a demonstration for Linking graphql's Flow Type with another packages,resolve type conflict will be easy,no more need to worry about how to make them 100% static resolved.

@itajaja
Copy link

itajaja commented Jan 18, 2017

any progress on this?

@stubailo
Copy link

Since this issue was filed, GraphQL.js 0.8 came out which exports Flow types. Perhaps the issue can be closed?

@leebyron
Copy link
Contributor

Yep, latest version has flow types!

@itajaja
Copy link

itajaja commented Jan 18, 2017

that's cool, thanks for closing this 🎉

@istarkov
Copy link

istarkov commented Feb 4, 2017

Hi, I get not covered by flow warnings in nuclide for every simple code with graphql-js

/* @flow */

import { GraphQLObjectType } from 'graphql';

const SomeType = new GraphQLObjectType({
  name: 'name',
});

And if I set name to 0 flow correctly gives me an error about type incompatibility.

The same I see in swapi-graphql package, so it does not looks like my config issues.
Can you help me explaining what to do and how to avoid this?

@wincent
Copy link
Contributor

wincent commented Feb 6, 2017

@istarkov: Mind filing this as a separate issue? And when you do, can you answer the following?

The same I see in swapi-graphql package, so it does not looks like my config issues.

@istarkov: I'm seeing flow check report 0 errors in the swapi-graphql repo. What do you get when you run npm run check (or yarn run check) there?

Thanks.

@istarkov
Copy link

istarkov commented Feb 7, 2017

@wincent It's not about flow check, it's about flow coverage
if you run flow coverage --color ./fileWithCodeFromAboveComment.js (looks like nuclide uses similar command to detect coverage) or view the coverage report inside nuclide you will see that everything inside GraphQLObjectType is not covered by flow.
I have no idea is it expected behaviour and having that I'm newbie in flow feel that I just doing something wrong.

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

No branches or pull requests

9 participants