add TS definitions #56

vutran · 2017-07-30T16:34:57Z

Initial draft of TS definitions as I'm planning to expand this more as I begin porting omnibar over to use the primitive components offered by react-autocompletely.

codecov-io · 2017-07-30T16:38:24Z

Codecov Report

Merging #56 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master    #56   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         262    262           
  Branches       63     63           
=====================================
  Hits          262    262

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8563d4b...29b6e24. Read the comment docs.

kentcdodds

This is super awesome!

I will merge this and I'm excited to do so with the understanding that I don't personally use typescript and do not plan to maintain TypeScript typings. If these fall out of maintenance and we start getting bug reports on them, I will not expend effort to fix them personally. It's not heartless, it's a matter of time.

So if you're willing to accept that, then I'm happy to merge this PR.

vutran · 2017-07-31T07:06:01Z

Of course. No expectations on your end. Hopefully the community can help build the TS definitions up alongside Flow in the other PR.

kentcdodds · 2017-07-31T12:16:33Z

Awesome! Thanks! If you don't mind, I'd like to wait until the API has settled a bit before we merge TypeScript typings. I don't want you to have to deal with at churn while we figure out what's best 👍

kentcdodds · 2017-08-08T04:11:20Z

Alright! I'm pretty confident the API has solidified, so if you'd like to update this PR that would be awesome 😄

Thanks!

vutran · 2017-08-09T01:56:05Z

@kentcdodds Not sure how you want tests for these definitions, but I made a repo here with the example provided in the README.

kentcdodds · 2017-08-09T04:06:51Z

For testing, maybe you could look at the glamorous repo. Maybe @luke-john could help guide this?

luke-john · 2017-08-09T15:26:46Z

Most projects simply test definitions by having a single test file that covers the api. ie.

// ./typings/add-numbers.d.ts
declare module 'add-two-numbers' {
    type AddTwoNumbers = (x: number, y: number) => x + y

    export default const addTwoNumbers: AddTwoNumbers
}

// ./test/add-numbers.ts
import addTwoNumbers from 'add-two-numbers'

const test: number = addTwoNumbers(1, 5)

With glamorous we took things a bit further to try and decrease the chance of regressions.

In addition to the above style test we also have a test/should-fail.ts which goes through the api and attempts to break things. ie.

// ./test/should-fail.ts
import addNumbers from 'add-numbers'

const test: number = addTwoNumbers(1)
const test: number = addTwoNumbers(1)
const test: number = addTwoNumbers('1', 5)
const test: number = addTwoNumbers(1, '5')
const test: number = addTwoNumbers(1, 5, 1)
const test: number = addTwoNumbers(1, 5, 1)

Given the current size of the typings just having a single test file is probably suitable.

It would be good to add a ~test:ts script to the package.json which runs typescript with the noEmit flag to test these.

Also if these typings are likely to grow in complexity you may want to consider landing them under a typings folder and adding "typings": "typings/index.d.ts",. This should make it easier to break into seperate files without polluting the top level folder.

luke-john · 2017-08-09T15:30:32Z

Just looking in the typings, it appears there's still a number of anys. This is fine, however you may want to add a note to the README that the typescript typings are still incomplete and contain anys to help set expectations for other users.

In Glamorous we added the following to a other/TYPESCRIPT_USAGE.md file and linked to this from the README.md.

# Typescript Usage

The current bundled typescript definitions are incomplete and based around the needs of the developers who contributed them.

Pull requests to improve them are welcome and appreciated. If you've never contributed to open source before, then you may find [this free video course](https://egghead.io/courses/how-to-contribute-to-an-open-source-project-on-github) helpful.

luke-john · 2017-08-09T15:37:23Z

index.d.ts

+    }
+
+    type ChildrenFunction = (options: ControllerStateAndHelpers) => React.ReactNode;
+    interface Downshift extends React.ComponentClass<Downshift.Props> { }


Having an interface named Downshift inside a Downshift namespace which passes the namespaces Props as Downshift.Props seems like it may cause confusion.

It may be simpler to keep everything at the top level. ie.

interface Props { ... } const downshift: React.ComponentClass<Props> export default downshift

Thanks for the suggestions! Will take note and address them :)

vutran · 2017-08-09T16:28:34Z

Most projects simply test definitions by having a single test file that covers the api. ie.

I was thinking of creating some test files based on the given examples but since it wouldn't make sense to place it into src/__tests__. I'm thinking src/__ts_tests__ but that may be a bit confusing to some users.

It would be good to add a ~test:ts script to the package.json which runs typescript with the noEmit flag to test these.

Will do.

Just looking in the typings, it appears there's still a number of anys. This is fine, however you may want to add a note to the README that the typescript typings are still incomplete and contain anys to help set expectations for other users.

I left the any types for items to start off with but will gladly introduce a generic for items.

kentcdodds · 2017-08-09T16:55:32Z

Thanks @luke-john!!

@vutran, if you could put the typescript test script in package-scripts under test that would be optimal. Then we can add it to the validate script to make it run concurrently 👍

vutran · 2017-08-15T17:33:32Z

@kentcdodds Just a quick update, will try to find some time this week to address the requested chances. (Apologies, trying to recover from outsidelands and work-related stuff in the same time).

kentcdodds · 2017-08-15T17:44:03Z

Sounds good. Thank you!

vutran · 2017-08-17T04:49:17Z

Okay, I think this is ready for a review. @luke-john

Removed Downshift namespace and have everything under the root namespace.
Export all interface to allow importing during usage.
Added tsc --noEmit test to package-scripts and added to the validate nps script.
Added other/TYPESCRIPT_USAGE.md
Moved typings to typings/index.d.ts

kentcdodds

Things look perfect as far as I can tell! Thank you so much for this!

luke-john · 2017-08-18T16:12:28Z

These are looking good to me @vutran. Great work 👍.

It looks like this project is using rollup in a similar way to glamorous so you may hit an issue with typescript not playing nice with the rollup compilation. If so the following addition to the entry file should solve things. paypal/glamorous#104

kentcdodds · 2017-08-18T16:15:42Z

Awesome! We're not on auto-release yet, but I'll get this published soon. Thanks so much @vutran! 🎉

vutran · 2017-08-19T01:16:00Z

It looks like this project is using rollup in a similar way to glamorous so you may hit an issue with typescript not playing nice with the rollup compilation. If so the following addition to the entry file should solve things. paypal/glamorous#104

I can open a new PR for this if needed.

kentcdodds · 2017-08-20T01:27:56Z

Feel free! I leave the TS stuff entirely up to other folks since I don't really use it 🤷‍♂️

kentcdodds · 2017-08-20T01:47:26Z

~/Developer/downshift (master)
🏎  $ npm publish --tag rc
+ downshift@1.0.0-rc.15

Let's try this out!

kentcdodds approved these changes Jul 31, 2017

View reviewed changes

vutran force-pushed the ts-definitions branch from 9bdac54 to 669831a Compare August 9, 2017 01:31

luke-john reviewed Aug 9, 2017

View reviewed changes

vutran and others added 4 commits August 16, 2017 21:33

add TS definitions

64e3d21

Rename to Downshift

a017e44

update with new API changes

ec02f85

update ts defintiions

cb82f7e

vutran force-pushed the ts-definitions branch from 47c0fde to cb82f7e Compare August 17, 2017 04:34

vutran added 3 commits August 16, 2017 21:37

fix pkg.json

dcbc426

import more typings

c7e21b1

move ts test to validate

29b6e24

kentcdodds approved these changes Aug 17, 2017

View reviewed changes

kentcdodds merged commit 033c6aa into downshift-js:master Aug 18, 2017

vutran deleted the ts-definitions branch August 18, 2017 19:51

vutran mentioned this pull request Aug 20, 2017

update entry file for ts consumption #144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add TS definitions #56

add TS definitions #56

vutran commented Jul 30, 2017

codecov-io commented Jul 30, 2017 •

edited

Loading

kentcdodds left a comment

vutran commented Jul 31, 2017

kentcdodds commented Jul 31, 2017

kentcdodds commented Aug 8, 2017

vutran commented Aug 9, 2017

kentcdodds commented Aug 9, 2017

luke-john commented Aug 9, 2017

luke-john commented Aug 9, 2017

luke-john Aug 9, 2017

vutran Aug 9, 2017

vutran commented Aug 9, 2017

kentcdodds commented Aug 9, 2017

vutran commented Aug 15, 2017

kentcdodds commented Aug 15, 2017

vutran commented Aug 17, 2017

kentcdodds left a comment

luke-john commented Aug 18, 2017

kentcdodds commented Aug 18, 2017

vutran commented Aug 19, 2017

kentcdodds commented Aug 20, 2017

kentcdodds commented Aug 20, 2017

add TS definitions #56

add TS definitions #56

Conversation

vutran commented Jul 30, 2017

codecov-io commented Jul 30, 2017 • edited Loading

Codecov Report

kentcdodds left a comment

Choose a reason for hiding this comment

vutran commented Jul 31, 2017

kentcdodds commented Jul 31, 2017

kentcdodds commented Aug 8, 2017

vutran commented Aug 9, 2017

kentcdodds commented Aug 9, 2017

luke-john commented Aug 9, 2017

luke-john commented Aug 9, 2017

luke-john Aug 9, 2017

Choose a reason for hiding this comment

vutran Aug 9, 2017

Choose a reason for hiding this comment

vutran commented Aug 9, 2017

kentcdodds commented Aug 9, 2017

vutran commented Aug 15, 2017

kentcdodds commented Aug 15, 2017

vutran commented Aug 17, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

luke-john commented Aug 18, 2017

kentcdodds commented Aug 18, 2017

vutran commented Aug 19, 2017

kentcdodds commented Aug 20, 2017

kentcdodds commented Aug 20, 2017

codecov-io commented Jul 30, 2017 •

edited

Loading