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 flow type signatures. #75

Closed
wants to merge 1 commit into from
Closed

Add flow type signatures. #75

wants to merge 1 commit into from

Conversation

Gozala
Copy link

@Gozala Gozala commented Jan 10, 2018

As dive into ipfs / libp2p code base I struggle to guess what are all the things passed around. Types as in flow (or typescript for that matter) simplify learning of how things are connected significantly. There for I decided to contribute type signatures to the code base as I digg through it.

What's up with all these .js.flow files ?

Unfortunately flow project has yet to come up with coherent story of how to provide type annotations for third party libraries. In my experience adding .js.flow files works best out of all options, that is because flow type checker when gives a precedent to /path/to/file.js.flow over /path/to/file.js which essentially provides a way to provide type annotations for non-flow typed package such that just using it as dependency will satisfy both type checker and run-time.

It's far from ideal given that .js and .js.flow files can get out of sync but then again unless you're open to adopt flow this is the best option IMO.

Depends on multiformats/js-multiaddr#51

@daviddias
Copy link
Member

@Gozala I hear your struggle. Having types annotation can help solve many production issues and as you well described, it is one of the best ways to document the code. Ideally, we can even use these annotations to generate the docs automatically. I like your proposal and where this is going.

It's far from ideal given that .js and .js.flow files can get out of sync but then again unless you're open to adopting flow this is the best option IMO.

Can't we add the flow checker as part of the CI to ensure that PR's only get merged if all the annotations are up to date?

@daviddias daviddias added status/ready Ready to be worked need/community-input Needs input from the wider community labels Feb 5, 2018
@carsonfarmer
Copy link
Contributor

Related to this, #110 adds typescript defs. These can be used to generate docs, and a CI step can be added to run the existing js tests through the typescript compiler as a type check step. The referenced PR includes the needed tooling for these, but does not add the CI step explicitly, nor does it update the docs creation -- which can be done with:
npx typedoc --mode file --includeDeclarations --excludeExternals ./src/index.d.ts

@vasco-santos
Copy link
Member

Closing this as we will not have flow types added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/community-input Needs input from the wider community status/ready Ready to be worked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants