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

generate flow types #676

Merged
merged 6 commits into from
Nov 28, 2016
Merged

generate flow types #676

merged 6 commits into from
Nov 28, 2016

Conversation

A-gambit
Copy link
Member

Issue #640

Provide flow types, using typedef-converter.
Please review, @mweststrate.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.663% when pulling 6000167 on flow into 7de9d72 on master.

@mweststrate
Copy link
Member

@A-gambit thanks!

I think the overloads for observable are the only pressing lacking here? It would be nice to add a test that invokes the flow compiler against a flow annotated javascript file and test whether it passes. Is that doable?

@A-gambit
Copy link
Member Author

A-gambit commented Nov 21, 2016

@mweststrate Yep. I will do it later on this week.

@A-gambit
Copy link
Member Author

A-gambit commented Nov 27, 2016

@mweststrate I review types and fix some bugs.
Unfortunately, flow doesn't work ok with the function which returns different types, like observable. Or action which can get different types of arguments. (https://flowtype.org/docs/functions.html#overloading-caution)
I spend to much time but doesn't get an appropriate result. But now we can use MobX observable interface in JavaScript code as types. And it is cool.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 95.665% when pulling 3b696f9 on flow into 7de9d72 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 95.665% when pulling 3b696f9 on flow into 7de9d72 on master.

@mweststrate
Copy link
Member

@A-gambit cool, thanks! So this is ready to be added as first version of flow support?

@@ -17,6 +17,7 @@
"test-browser-safari": "npm run small-build && ( browserify test/*.js -t [ babelify --presets [ es2015 ] ] | tape-run --browser safari )",
"test-browser-firefox": "npm run small-build && ( browserify test/*.js | tape-run --browser firefox )",
"test-travis": "npm run small-build && npm run build-tests && tape test/*.js test/perf/index.js && tsc && istanbul cover tape test/*.js",
"test-flow": "node_modules/.bin/flow check",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should be invoked from test-travis and full-test

@A-gambit
Copy link
Member Author

@mweststrate Sure. I have just add flow check to test scripts.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 95.665% when pulling 463b96e on flow into 7de9d72 on master.

@A-gambit
Copy link
Member Author

A-gambit commented Nov 28, 2016

@mweststrate So, I can merge?

@mweststrate
Copy link
Member

mweststrate commented Nov 28, 2016 via email

@A-gambit
Copy link
Member Author

@mweststrate Ok, I will merge it to v3, and add new v3 methods to flow types. I think it will be better.

@A-gambit A-gambit changed the base branch from master to mobx3 November 28, 2016 15:31
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.115% when pulling a8dcdad on flow into ee8441c on mobx3.

@A-gambit
Copy link
Member Author

@mweststrate Done. Can I merge?

@mweststrate
Copy link
Member

mweststrate commented Nov 28, 2016 via email

@A-gambit A-gambit merged commit c3ed90e into mobx3 Nov 28, 2016
@A-gambit A-gambit deleted the flow branch November 28, 2016 17:06
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.

3 participants