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

feat(core): Add flow types, flow build step, type tests #21

Merged
merged 6 commits into from
Jan 17, 2017

Conversation

briancavalier
Copy link
Member

Add flow type definitions for the public API. The goal is to provide near term type safety value for consumers. This doesn't touch the much larger task of adding type annotations to the implementation.

What's here:

  • a complete set of type definitions for the public API in src/index.js.flow
  • a build step that copies the type definitions to the correctly-named files in the dist/ folder, so that flow will find them in the npm package
  • an infrastructure for type checking tests and one simple test to start with. The files in test/flow will be flow checked by npm test, and it will fail if type checking fails. Fail CI if types are invalid FTW
  • linting via eslint babel-eslint for the flow type definitions and the type tests

What's (intentionally) not here:

  • type annotations in implementation files

Interesting or controversial things we may want to discuss:

  1. I chose to define types for Scheduler, Timeline, and Timer, rather than classes. Defining classes in flow would require a complete definition, including implementation details like private instance properties and methods. Defining types only requires the public API to be defined.
    • It seems unlikely, but this could cause confusion of the Scheduler type vs. Scheduler class for someone inspecting a Scheduler class instance in a debugger. One way to mitigate this would be to rename the Scheduler class.
    • This prevents subclassing. Personally, I don't see this as an issue, and I'd rather not promote subclassing these things. Scheduler is parameterized for a reason, and if you're going to write your own Scheduler, it's unlikely that subclassing will help you in any meaningful way.
  2. Because of 1, I decided to export smart constructors which return values of the corresponding type. For example , e.g. newScheduler (timer: Timer, timeline: Timeline): Scheduler, which returns something of the type Scheduler.
    • I went with "newFoo" as the naming scheme here. I'm open to other options, like "createFoo", "makeFoo", etc., but "newFoo" is nice and short and says exactly what it does.

@briancavalier briancavalier mentioned this pull request Jan 13, 2017
@briancavalier
Copy link
Member Author

This is currently using the dist flow files to run the type checking tests. I'm not sure if that's the right thing. They're just copies of src/index.js.flow, so maybe the tests should just run against that. Is this a part of my secret plan to remove the dist folder from the repo? perhaps ...

@briancavalier
Copy link
Member Author

Ok, I added types for combine, combineArray, zip, and zipArray. Varargs combine, and zip weren't too bad. I ran into a question about combineArray and zipArray, though. I wasn't sure whether to provide a "catch-all" version that allows arbitrary length arrays at the cost of giving up type checking on the array, see here. I saw that xstream does the same. I have no idea if there's a better way in flow, but I can't think of one.

I went to arity 6. It's purely mechanical to go higher if we feel like we should. Opinions?

@briancavalier
Copy link
Member Author

I think this is ready for review. We may want to expand the type tests, but also may want to do that in another PR in order to get this merged sooner. Totally open to discussion, tho.

@briancavalier
Copy link
Member Author

Rebased

dispose: () => ?Promise<any>
}

// A Timer can scheduler a function to run after
Copy link
Member

@TylorS TylorS Jan 16, 2017

Choose a reason for hiding this comment

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

minor typo scheduler -> schedule. Silly, I know 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

D'oh! fixed

@briancavalier briancavalier merged this pull request into master Jan 17, 2017
@briancavalier briancavalier deleted the add-api-flow-types branch January 17, 2017 00:38
briancavalier added a commit that referenced this pull request Jan 17, 2017
AFFECTS: @most/core

* Add flow types, build step, type tests

* Add missing flow-bin devDep

* Add sample, combine, combineArray, zip, zipArray

* Run type check tests against src

* chore(dist): Re-remove dist files

AFFECTS: @most/core

* docs(index.js.flow): Fix typo

AFFECTS: @most/core
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants