Types 1.2.0 #165

tansongyang · 2017-08-31T18:06:37Z

What: Update typings.

Why: Some types did not match the docs.

How: Edit index.d.ts

Checklist:

Documentation N/A
Tests
Ready to be merged
Added myself to contributors table

I tried to make only non-breaking changes (adding types, making required types optional, etc.) with one exception. GetButtonPropsOptions was requiring incorrect properties—in fact, it shouldn't be requiring any at all. I completely removed them.

There are a couple things that probably should change, but I didn't want to make them for fear of introducing a breaking change. These are:

The props onUserAction and onClick in DownshiftProps are not in the docs. Have they been removed? If so, they should be removed from the type definitions as well.
A11StatusMessageOptions should probably be renamed to A11yStatusMessageOptions (there is a missing "y")
ChangeOptions is completely unused, and I could not find any mention of it in the docs. However, it is exported, so I did not remove it in case anyone is importing it.

These props are not required for getButtonProps.

Allow null in places where the docs say they can go. Add missing `itemCount` property.

codecov-io · 2017-08-31T18:09:47Z

Codecov Report

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

@@          Coverage Diff          @@
##           master   #165   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         267    267           
  Branches       64     64           
=====================================
  Hits          267    267

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 47ee3b8...4f334e0. Read the comment docs.

kentcdodds

Looks awesome to me! I'd love a review from some other folks though...

kentcdodds · 2017-08-31T20:18:26Z

ping @pbomb and @vutran could you review this?

pbomb · 2017-08-31T20:35:42Z

typings/index.d.ts

@@ -1,25 +1,28 @@
 import * as React from 'react';

+type CB = () => void


The callbacks are optional. Additionally, the code checks if they are functions and only returns them if they are. So, I think it would be more correct to go with either Function | undefined or () => any | undefined. I think void is too restrictive - we don't really care what the callback function returns.

The TypeScript types for React's setState function (which is how these callbacks are used) is typed as () => any | undefined

pbomb · 2017-08-31T20:36:36Z

typings/index.d.ts

    defaultSelectedItem?: any;
    defaultInputValue?: string;
    defaultIsOpen?: boolean;
    getA11yStatusMessage?: (options: A11StatusMessageOptions) => any;
    itemToString?: (item: any) => string;
    onChange?: (selectedItem: any, stateAndHelpers: ControllerStateAndHelpers) => void;
-    onStateChange?: (options: StateChangeOptions, stateAndHelpers: ControllerStateAndHelpers) => void;
+    onStateChange?: (changes: StateChangeOptions, stateAndHelpers: ControllerStateAndHelpers) => void;


I prefer to keep this named as options to be consistent with everything else in here.

pbomb · 2017-08-31T20:38:26Z

typings/index.d.ts

+    openMenu: (cb: CB) => void;
+    closeMenu: (cb: CB) => void;
+    toggleMenu: (cb: CB) => void;
+    selectItem: (item: any, otherStateToSet: any, cb: CB) => void;


otherStateToSet and cb should be optional types. Additionally, if otherStateToSet is defined, it should be an object as it gets spread into another object.

pbomb · 2017-08-31T20:38:53Z

typings/index.d.ts

+    closeMenu: (cb: CB) => void;
+    toggleMenu: (cb: CB) => void;
+    selectItem: (item: any, otherStateToSet: any, cb: CB) => void;
+    selectItemAtIndex: (index: number, otherStateToSet: any, cb: CB) => void;


otherStateToSet and cb should be optional types. Additionally, if otherStateToSet is defined, it should be an object as it gets spread into another object.

pbomb · 2017-08-31T20:38:58Z

typings/index.d.ts

+    toggleMenu: (cb: CB) => void;
+    selectItem: (item: any, otherStateToSet: any, cb: CB) => void;
+    selectItemAtIndex: (index: number, otherStateToSet: any, cb: CB) => void;
+    selectHighlightedItem: (otherStateToSet: any, cb: CB) => void;


otherStateToSet and cb should be optional types. Additionally, if otherStateToSet is defined, it should be an object as it gets spread into another object.

pbomb · 2017-08-31T20:39:42Z

typings/index.d.ts

+    selectItem: (item: any, otherStateToSet: any, cb: CB) => void;
+    selectItemAtIndex: (index: number, otherStateToSet: any, cb: CB) => void;
+    selectHighlightedItem: (otherStateToSet: any, cb: CB) => void;
+    setHighlightedItem: (index: number, otherStateToSet: any, cb: CB) => void;


otherStateToSet should be optional and there is no cb argument to this function.

Are you talking about setHighlightedItem here? If so, I think that, first of all, this must be a typo; I just noticed that setHighlightedItem is not in the docs. However, there is setHighlightedIndex. If that is the case, I believe it does have a cb argument:

setHighlightedIndex function(index: number, otherStateToSet: object, cb: Function) call to set a new highlighted index

Hmm, I was talking about setHighlightedItem, but now I don't see it anywhere in the repo except in the TypeScript definitions, so I think you can remove this line.
As for the line above, with selectHighlightedItem, you're right. otherStateToSet should be optional and there is also an optional cb argument.

pbomb · 2017-08-31T20:39:58Z

typings/index.d.ts

+    selectItemAtIndex: (index: number, otherStateToSet: any, cb: CB) => void;
+    selectHighlightedItem: (otherStateToSet: any, cb: CB) => void;
+    setHighlightedItem: (index: number, otherStateToSet: any, cb: CB) => void;
+    clearSelection: (cb: CB) => void;


cb is optional

pbomb · 2017-08-31T20:40:46Z

typings/index.d.ts

+    selectHighlightedItem: (otherStateToSet: any, cb: CB) => void;
+    setHighlightedItem: (index: number, otherStateToSet: any, cb: CB) => void;
+    clearSelection: (cb: CB) => void;
+    reset: (otherStateToSet: any, cb: CB) => void;


cb is optional

Is otherStateToSet also optional here?

tansongyang · 2017-09-02T01:08:46Z

@pbomb First of all, thanks for reviewing the PR! I have a local commit with the changes, but I'm waiting to push until we resolve my two follow-up questions. Also, do you have any thoughts on my additional comments (in the original PR post)? If you think any of those are worth doing, we can also do them in this branch so that it all gets merged in on one go.

* Rename `changes` back to `options`. * Add optional arguments in the right places. * Use `Function` as type for `cb`. * Use `object` ast type for `otherStateToSet`. * Fix incorrect name `setHighlightedItem` by changing it to `setHighlightedIndex`.

pbomb · 2017-09-02T15:08:33Z

@tansongyang sorry, i missed your questions in the initial PR posting. Here's what my thoughts are.

You're right, there does seem to be an optional onClick prop that isn't documented. @kentcdodds does documentation need to be added for this prop? It looks to me like the user can pass onClick both to the <Downshift/> component and to the top-level element returned by the function-as-child and that they will both be called as the click handlers are all composed together. Does that sound right to you?
Yes, A11StatusMessageOptions should be renamed to A11yStatusMessageOptions as it describes the argument to the getA11yStatusMessage function prop.
You should remove ChangeOptions as it's out of date. That used to be the type of the first argument passed to the onChange callback prop, but was changed to just be the selectedItem in commit 620a3ad5e4f50dece867ae9989174a8b35104fb3 and previousItem was removed in that commit as well.

kentcdodds · 2017-09-03T04:21:50Z

You're right, there does seem to be an optional onClick prop that isn't documented. @kentcdodds does documentation need to be added for this prop? It looks to me like the user can pass onClick both to the component and to the top-level element returned by the function-as-child and that they will both be called as the click handlers are all composed together. Does that sound right to you?

That's a mistake actually. We should remove the prop. It's undocumented so I don't think we need a breaking change for it. Sorry about that!

* Rename `A11StatusMessageOptions` to `A11yStatusMessageOptions`. * Remove `ChangeOptions`. * Remove `onClick` prop.

tansongyang · 2017-09-03T15:48:37Z

I've made the changes that we discussed. Let me know if anything else needs to change.

pbomb

This looks great. Thanks @tansongyang!

kentcdodds

Thank you so much!

kentcdodds · 2017-09-03T21:50:32Z

There are merge conflicts. Could you fix them? Thanks!

tansongyang · 2017-09-05T12:46:29Z

@kentcdodds Thanks for getting that README merge conflict! It was a pleasure working with you all.

kentcdodds · 2017-09-05T13:34:30Z

You're welcome!

The build is broken right now due to a problem with TypeScript. Could someone look into that please?

pbomb · 2017-09-05T13:40:10Z

I'm taking a look now

tansongyang · 2017-09-05T13:47:54Z

Found the issue. After I removed ChangeOptions, I forgot to update the tests, which reference ChangeOptions. That's my bad. Working on a fix right now.

tansongyang · 2017-09-05T13:55:04Z

In my local environment, I believe I've fixed the TS issue, as it now successfully compiles. However, npm start validate reports lint issues with downshift\stories\examples\multiple.js. What would you like to do about that?

Also, the problem in the test files was custom.test.tsx and basic.test.tsx were importing ChangeOptions, but they weren't using it. Removing it from the imports fixes the issue.

kentcdodds · 2017-09-05T13:57:34Z

Go ahead and fix whatever you can and I'll fix what's left. Thanks!

tansongyang · 2017-09-05T14:03:17Z

New pull request: #175

Apologies for the inconvenience. Thanks for being flexible.

tansongyang added 7 commits August 31, 2017 12:45

Update actions types

8493636

Remove props from GetButtonPropsOptions

c15dcdc

These props are not required for getButtonProps.

Make getItemProps index optional

b433e21

Update state types

ef6b708

Update DownshiftProps and A11StatusMessageOptions

e82843f

Allow null in places where the docs say they can go. Add missing `itemCount` property.

Remove version number from types

8bf8342

Update contributors

4f334e0

kentcdodds approved these changes Aug 31, 2017

View reviewed changes

pbomb reviewed Aug 31, 2017

View reviewed changes

Address PR comments

c53b19e

* Rename `changes` back to `options`. * Add optional arguments in the right places. * Use `Function` as type for `cb`. * Use `object` ast type for `otherStateToSet`. * Fix incorrect name `setHighlightedItem` by changing it to `setHighlightedIndex`.

Address updated PR comments

22e90f8

* Rename `A11StatusMessageOptions` to `A11yStatusMessageOptions`. * Remove `ChangeOptions`. * Remove `onClick` prop.

pbomb approved these changes Sep 3, 2017

View reviewed changes

kentcdodds approved these changes Sep 3, 2017

View reviewed changes

Kent C. Dodds added 2 commits September 3, 2017 19:04

Merge branch 'master' into types-1.2.0

a738db5

Update README.md

35bae68

kentcdodds approved these changes Sep 4, 2017

View reviewed changes

kentcdodds merged commit 0f71611 into downshift-js:master Sep 4, 2017

tansongyang mentioned this pull request Sep 5, 2017

Fix TS build issue #175

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Types 1.2.0 #165

Types 1.2.0 #165

tansongyang commented Aug 31, 2017

codecov-io commented Aug 31, 2017

kentcdodds left a comment

kentcdodds commented Aug 31, 2017

pbomb Aug 31, 2017

pbomb Aug 31, 2017

pbomb Aug 31, 2017

pbomb Aug 31, 2017

pbomb Aug 31, 2017

pbomb Aug 31, 2017

tansongyang Sep 2, 2017 •

edited

Loading

pbomb Sep 2, 2017

pbomb Aug 31, 2017

pbomb Aug 31, 2017

tansongyang Sep 2, 2017

tansongyang commented Sep 2, 2017

pbomb commented Sep 2, 2017

kentcdodds commented Sep 3, 2017

tansongyang commented Sep 3, 2017

pbomb left a comment

kentcdodds left a comment

kentcdodds commented Sep 3, 2017

tansongyang commented Sep 5, 2017

kentcdodds commented Sep 5, 2017

pbomb commented Sep 5, 2017

tansongyang commented Sep 5, 2017

tansongyang commented Sep 5, 2017

kentcdodds commented Sep 5, 2017

tansongyang commented Sep 5, 2017

		@@ -1,25 +1,28 @@
		import * as React from 'react';

		type CB = () => void

Types 1.2.0 #165

Types 1.2.0 #165

Conversation

tansongyang commented Aug 31, 2017

codecov-io commented Aug 31, 2017

Codecov Report

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tansongyang Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tansongyang commented Sep 2, 2017

pbomb commented Sep 2, 2017

kentcdodds commented Sep 3, 2017

tansongyang commented Sep 3, 2017

pbomb left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Sep 3, 2017

tansongyang commented Sep 5, 2017

kentcdodds commented Sep 5, 2017

pbomb commented Sep 5, 2017

tansongyang commented Sep 5, 2017

tansongyang commented Sep 5, 2017

kentcdodds commented Sep 5, 2017

tansongyang commented Sep 5, 2017

tansongyang Sep 2, 2017 •

edited

Loading