Conversation
|
Awesome. I'll see it lator.
It is difficult choice. I'm not familiar with FlowType. |
azu
left a comment
There was a problem hiding this comment.
Currenly, UseCaseExecutor is private class.
But, we should public this.
type-definitions/almin.js.flow
Outdated
| Store, | ||
| StoreGroup, | ||
| QueuedStoreGroup, | ||
| UseCaseExecutor, |
There was a problem hiding this comment.
📝 Currently, UseCaseExecutor stay in the closet.
It means that UseCaseExecutor is under the unstable. It is potential to change.
But, I know that type definition(d.ts and flow) need to public the internal type because it can be touched from outside.
I forget correctly JSDoc typing of UseCaseExecutor.
+ onWillExecuteEachUseCase(handler: (useCase: UseCase, args: mixed) => mixed): void;
+ onDidExecuteEachUseCase(handler: (useCase: UseCase) => mixed): void;
+ onCompleteExecuteEachUseCase(handler: (useCase: UseCase) => mixed): Function;
+ execute(args: Array<mixed>): void;
+ release(): void;are public.
+ willExecute(args: Array<mixed>): void;
+ didExecute(): void;
+ complete(): void;are private.
There was a problem hiding this comment.
Omit private function definitions and make UseCaseExecutor class unexportable as type.
Thanks for your review! |
|
We have three options: Merge
and We want to add @yamadayuki as Collabolator. Not Merge
|
|
I actually think option 1 and option 2 are nearly same. I hope to choose option 2. Concerning option 3, I suppose that the flow-typed is still unpopular among the developers who use flow type. And in the flow-typed repository, I will send pull request such as this PR, reviewers consider about the type definition to be valid. But they don't have the responsibility for a behavior of the module. So I'd rather not choose option 3. |
|
@yamadayuki OK. I choose option 2. |
azu
left a comment
There was a problem hiding this comment.
almin/package.json use files field.
It is a whitelist for npm publishing.
Should we added type-definitions to files?
example/todomvc-flow/package.json
Outdated
| "build": "NODE_ENV=production webpack -p", | ||
| "watch": "NODE_ENV=development webpack -d -w", | ||
| "prepublish": "npm run --if-present build", | ||
| "test": "mocha" |
There was a problem hiding this comment.
Can you add flow check script?
It means that CI always test type definition file and example code.
"scripts": {
"test": "mocha && npm run flow"
"flow": "flow check"
}
// via https://flowtype.org/blog/2016/08/01/Windows-Support.html
DefinitelyTyped of TypeScript has a set of d.ts and test code for d.ts.
I think that example project and flow check is instead of this.
example/todomvc-flow/.flowconfig
Outdated
| ../../src/ | ||
|
|
||
| [libs] | ||
| ../../type-definition |
There was a problem hiding this comment.
Sorry, I forgot to add this correction.. Fixed.
OK. |
example/todomvc-flow/package.json
Outdated
| "webpack-dev-server": "^2.0.0-beta" | ||
| }, | ||
| "bin": { | ||
| "flow": "./node_modules/.bin/flow" |
There was a problem hiding this comment.
I belive that it is not necessary.
"bin" field is for publishing
https://docs.npmjs.com/files/package.json#bin
npm run-script automatically add $(npm bin) path to PATH enviroment variable.
There was a problem hiding this comment.
npm run-script automatically add $(npm bin) path to PATH enviroment variable
I misunderstanding about this. Remove it. Thank you for your advice.
|
@azu Thank you anyway! Sorry for my a lot of flaws. |
|
@yamadayuki No problem! Thanks for PR. I've released https://github.com/almin/almin/releases/tag/0.9.0 and added @yamadayuki as collaborator 🎉 |
For #55 .
Add
type-definitions/almin.js.flow. It enables developers to build a large application using almin with Flow type checker. I added TodoMVC example with flow.I use the Facebook's immutable-js as a reference to make definition directory.