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

Add TypeScript declaration files #82

Merged
merged 7 commits into from Aug 30, 2017
Merged

Add TypeScript declaration files #82

merged 7 commits into from Aug 30, 2017

Conversation

toverux
Copy link
Contributor

@toverux toverux commented Aug 29, 2017

Hello @nodkz,

This is the PR associated to our recent discussion in #81.
It's been easier than I thought due to the feature and syntactical similarities between Flow and TypeScript.

So I basically ported the public API, including the types API, of course. Flow-specific types can be found in their 1:1 equivalents in the TypeScript version.

A few notes :

  • This is a first version, and like I said, a rapid port from Flow. A few methods/arguments/props/etc could probably be better typed using advanced TypeScript static typing features (a bit like in is.d.ts where I've added type guards in lieu of the original booleans at the latest minute).

    This could be improved progressively as TypeScript users work with those declarations (probably me, heh)

  • I've decided to replicate the filesystem structure of src/ in types/ (community convention). This could have been merged with src/ too, or that could have been a single file of hundreds of LoCs. I thought this would help for maintenance as you don't have to search for long to find the TypeScript counterparts.

  • You'll see that not every .js file has its .d.ts counterpart, as I said, I've only ported the public API.

  • The commented types in definition.js beginning with "Flow 0.47 not ready for this" have been enabled in the TS version.

  • Suggestion: In the stub I was using before redacting this new, improved version, I was exporting alot of types, like for example ResolveParams<>. I see that all of those wonderful types aren't exported by the main module, meaning I have to do import { ResolveParams } from 'graphql-compose/types/definition';.

    Could we export the whole content of definition.{js|d.ts} on the main module, or do you want to keep them semi-private for now?

That's pretty much everything for now! :)

I've added two npm scripts: one that lints the .d.ts files, and one that calls the TypeScript compiler to perform type-checking. They are also called in the existing lint and test scripts.

Tested against one of my codebases (cp -r graphql-compose ../myproject/node_modules as linking packages seems to still be an experimental, non-deterministic technology..), works flawlessly (I've even catched a 2.0 deprecation thanks to it!).

Let me know if you have questions, remarks or anything, before merging that 😃

@nodkz
Copy link
Member

nodkz commented Aug 30, 2017

Brilliant PR. Thanks a lot. 👍
Merging immediately!

Could we export the whole content of definition.{js|d.ts} on the main module, or do you want to keep them semi-private for now?

Yep, we can export them. In same way now React exports its internal types (note about import * as React from 'react'). Anyway, let's move this improvement to another PR.

@nodkz nodkz merged commit cb9678e into graphql-compose:master Aug 30, 2017
@nodkz
Copy link
Member

nodkz commented Aug 30, 2017

Bumped 2.4.0. @toverux please check that published package works with our codebase.

@toverux
Copy link
Contributor Author

toverux commented Aug 30, 2017

@nodkz awesome! Thanks a lot. I'll watch the repo in the case my help is needed when the API changes.
I can confirm that 2.4.0 works fine!

I'll make the other PR later. I presume that we'll export those types on the index? Is there other exports you'd want to change or add?

@nodkz
Copy link
Member

nodkz commented Sep 6, 2017

@toverux please review TypeScript defs in refactor_definition branch https://github.com/nodkz/graphql-compose/tree/refactor_definition
I removed definition.js file and put all type declarations in its owner files. All checks pass, but maybe something broken with your app.

Tomorrow I'll want to migrate all graphql-compose-plugins. If you found some errors or suggestions let me know. Sorry for hurry, but I blocked in my internal app development. So I want to complete all changes fastly. If you have no time, don't worry, we can fix TS errors in next versions (of course if errors exist 😈).

@toverux
Copy link
Contributor Author

toverux commented Sep 6, 2017

Hi @nodkz, I'll do a quick check tomorrow (I mean, this morning, it's 1:30 AM here) asap!
(By "quick", I mean that for now the 3 apps that use graphql-compose are still small and only one uses the version that embeds the type defs, so I can't guarantee everything is fine just by compiling).

I also have a meeting at 9AM so, I don't want to slow you down, if you don't get any news from me soon enough, feel free to make your refactors and I'll check afterwards ;)

(btw: the latest releases are cool! o/)

@toverux
Copy link
Contributor Author

toverux commented Sep 7, 2017

A bit late but - it seems good!

@nodkz
Copy link
Member

nodkz commented Sep 13, 2017

Thanks for help!

One notice, when I'm worked with types folder it blows a little bit my mind (especially types/type/...) and also I several times forgot to change declarations.

Can we put declarations near source files without duplicating directory structure? It will be much easy to maintain. Eg. if changed src/resolver.js so it easy to not miss changes for src/resolver.d.ts.

Do you have time to make such PR and test that these changes work with your code? Thanks.

@toverux
Copy link
Contributor Author

toverux commented Sep 13, 2017

Hi @nodkz,

Yeah, I mentioned that point when I did the PR. It's common to find the .d.ts files in types/ when different persons maintain the typings and the source. However, letting the .d.ts aside of the .js files causes no problem at all, and at the contrary, it's often simpler!

I can find time to make a PR in the next few hours I think ;)

PS: we've just started another project that uses graphql-compose, it has kind of became as fundamental as express or mongoose now - such a pain killer - thanks for the job!

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

Successfully merging this pull request may close these issues.

None yet

2 participants