fix(hooks): add type to on change handler params #985

silviuaavram · 2020-04-04T15:59:34Z

What:
As discussed here we are adding type to other handlers as we claim to support this already in the docs.

Why:

We already claim to support these. We already pass type in onStateChange.
Fixes #1015.

How:

Add type in other chance handlers as well (onIsOpenChange, onSelectedItemChange etc.)
Fix TS typings for these handlers to reflect they can have partial state + type.
Remove props from the stateReducer call param actionAndChanges. Now it should be called with {...action, changes}, where action has a type and other data passed by the hook in dispatch.
Also fix typings for stateReducer in both hooks to reflect it's being called with {...action, changes}.

Checklist:

codecov-io · 2020-04-04T16:29:34Z

Codecov Report

Merging #985 into master will not change coverage by %.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #985   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines         1022      1024    +2     
  Branches       200       200           
=========================================
+ Hits          1022      1024    +2

Impacted Files	Coverage Δ
src/hooks/utils.js	`100.00% <100.00%> (ø)`

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 449f3f0...6f449bb. Read the comment docs.

codecov-commenter · 2020-05-31T16:06:33Z

Codecov Report

Merging #985 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #985   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         1236      1237    +1     
  Branches       239       239           
=========================================
+ Hits          1236      1237    +1

Impacted Files	Coverage Δ
src/hooks/utils.js	`100.00% <100.00%> (ø)`

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 de3da1e...342c4d7. Read the comment docs.

kimroen · 2020-06-06T22:26:52Z

I actually just went in to fix this myself and saw this PR. What's holding this back?

I implemented the fix in a very similar way, but as far as I could tell the type property is already passed in to the onChange handlers. Am I mistaken?

How is this a breaking change, by the way? I wouldn't think adding a property would break anything.

silviuaavram · 2020-06-07T15:46:01Z

Hi @kimroen! It's breaking as a result of the TypeScript changes. It's already passed to onStateChange yes, but not to the others, like onHighlightedIndexChange.

I am waiting to converge a few more PRs with breaking changes before releasing v6. But I don't think that this PR is blocked by anything, as far as I see it it should be ready to be merged. You can review it also and tell me what you think, on both the functionality change and the Typescript changes. Thank you!

kimroen · 2020-06-07T17:31:41Z

It's breaking as a result of the TypeScript changes. It's already passed to onStateChange yes, but not to the others, like onHighlightedIndexChange.

Thanks for the explanation! I don't think I agree that adding a field to an object passed to a function is a breaking change, but I'm not extremely invested and I can go along with it ✨

Regarding the changes, I made pretty much the same change in my own fix, here:

kimroen@c7b1062

In addition to that one though, I actually made another change which I would love your thoughts on:

kimroen@c7d2264

This makes it so the field that is relevant for the specific change callback function it is passed to is not optional.

In other words, in the change passed to onInputValueChange, the inputValue field is non-optional.

In practice this looks like this:

(Note which fields are optional in the two)

Is this something we could be interested in including? If so, I could make a PR for it, but it would conflict pretty heavily with this one, so I wanted to ask 😄

MikeAliG · 2020-06-11T11:08:34Z

Any update on this?

silviuaavram · 2020-06-13T14:43:28Z

It's not just adding a field, it's replacing as well, for instance:
onInputValueChange?: (changes: Partial<UseComboboxState<Item>>) => void
with
onInputValueChange?: (changes: UseComboboxStateChange<Item>) => void
which is different (and correct).

As for status, please review this @AliTheAli @kimroen and make sure it's ready to merge. It will be included in the next major release. I want to merge a few more breaking change PRs into it so that is why I am not merging it right now.

kimroen

Regarding this comment:

It's not just adding a field, it's replacing as well, for instance:
onInputValueChange?: (changes: Partial<UseComboboxState>) => void
with
onSelectedItemChange?: (changes: Partial<UseComboboxState>) => void
which is different (and correct).

I don't see this change in your PR - are you saying you renamed onInputValueChange to onSelectedItemChange, or is there another difference here that I'm supposed to be seeing?

kimroen · 2020-06-16T08:04:03Z

typings/index.d.ts

+
+export interface UseSelectDispatchAction {
+  type: UseSelectStateChangeTypes
+  [data: string]: any


This key that has a key of string and a value of any is going to make autocomplete a little less useful - do we not have anything more complete we could put here instead, or is it entirely user-defined?

Oups, edited with the acutal line of onInputValueChange.

That line with [data:string]: any is because the dispatch may have stuff in it required to change the state. Do you have a better suggestion?

If we don't know what that "stuff" is ahead of time then it might be okay, but if we do then we should try to be more specific in the type. If we don't know, then it would be better (for typescript) if all the unknown fields are in a separate object so we can limit the unknowns to one place without polluting the main change object.

I think we can determine what the fields are by going through all the dispatch calls in the hooks and add those fields as optional. Would that be ok?

silviuaavram · 2020-06-19T12:57:10Z

@kimroen thank you for suggesting to update the types for action. How do they look now? :D

kimroen · 2020-06-29T08:21:16Z

@kimroen thank you for suggesting to update the types for action. How do they look now? :D

This is so much better, thank you very much! ✨

**What**: Update downshift to v6. **Why**: Introduce breaking changes that fix the issues below. Fixes #1088. Fixes #1015. Fixes #1010. Fixes #719. **How**: **The list of breaking changes:** BREAKING CHANGE: Update TS typings for `selectedItem` to accept `null` in both `useSelect` and `useCombobox`. To migrate to the new change, update your types or code if necessary. `selectedItem`, `defaultSelectedItem` and `initialSelectedItem` now have `Item | null` instead of `Item` type. PR with the changes: #1090 BREAKING CHANGE: Update TS typings for `itemToString` to accept `null` for the `item` parameter, in `useSelect` and `useCombobox` + in `Downshift` where this was missing. `useMultipleSelection` type for `itemToString` stays the same as it can't receive `null` as `item`. To migrate to the new change, update your types or code if necessary. `itemToString: (item: Item) => string` -> `itemToString: (item: Item | null) => string}`. PR with the changes: #1075 #1105 BREAKING CHANGE: Pass `type` to the onChange (onInputValueChange, onHighlightedIndexChange, onSelectedItemChange, onIsOpenChange) handler parameters, as specified in the documentation. Also updated the TS typings to reflect this + `onStateChange` - the `type` parameter was passed but it was not reflected in the TS types. To migrate to the new change, update your types or code if necessary, better to view the changes in the PR: #985. BREAKING BEHAVIOUR [useCombobox]: When an item is highlighted by keyboard and user closes the menu using mouse/touch, the item is not selected anymore. The only selection on Blur happens using either Tab / Shift+Tab. PR with the changes: #1109 BREAKING BEHAVIOUR [useCombobox & downshift]: When pressing Escape and the menu is open, only close the menu. When the menu is closed and there is an item selected and/or text in the input, clear the selectedItem and the inputValue. PR with the changes: #719

silviuaavram added bug TypeScript labels Apr 4, 2020

silviuaavram mentioned this pull request Apr 4, 2020

feat: multi-select hook for useSelect and useCombobox #879

Closed

silviuaavram added needs primary reviewer attention and removed needs primary reviewer attention labels Apr 4, 2020

silviuaavram mentioned this pull request Apr 5, 2020

hooks: pass Event in the onChange handlers #986

Closed

silviuaavram added the BREAKING CHANGE label Apr 7, 2020

silviuaavram mentioned this pull request May 8, 2020

[Typings] Field type missing from useCombobox / useSelect 's props.onStateChange #1015

Closed

kimroen reviewed Jun 16, 2020

View reviewed changes

silviuaavram added 5 commits June 27, 2020 15:03

include type in hook on change handler params

c1583d7

fix tests to check this change

78757b3

update TS typings

66811df

fix ts typings again

77c5add

add typings for action

828d0e2

silviuaavram force-pushed the fix/on-change-handler-params branch from fcf116e to 828d0e2 Compare June 27, 2020 12:05

silviuaavram changed the base branch from master to v6 June 27, 2020 12:11

silviuaavram merged commit 5f43e78 into v6 Jun 27, 2020

silviuaavram deleted the fix/on-change-handler-params branch June 27, 2020 12:12

silviuaavram mentioned this pull request Jun 27, 2020

downshift v6 #1104

Merged

5 tasks

renovate bot mentioned this pull request Jun 6, 2021

Update dependency downshift to v6 - autoclosed adsanderson/xstate-test-test#16

Closed

1 task

renovate bot mentioned this pull request Jun 6, 2021

fix(deps): update dependency downshift to v6 - autoclosed Raigen/storeroom-firebase#218

Closed

1 task

dakahn mentioned this pull request May 3, 2022

chore(Downshift): update dependency to latest carbon-design-system/carbon#11331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(hooks): add type to on change handler params #985

fix(hooks): add type to on change handler params #985

silviuaavram commented Apr 4, 2020 •

edited

Loading

codecov-io commented Apr 4, 2020

codecov-commenter commented May 31, 2020

kimroen commented Jun 6, 2020

silviuaavram commented Jun 7, 2020

kimroen commented Jun 7, 2020

MikeAliG commented Jun 11, 2020

silviuaavram commented Jun 13, 2020 •

edited

Loading

kimroen left a comment

kimroen Jun 16, 2020

silviuaavram Jun 16, 2020

kimroen Jun 16, 2020

silviuaavram Jun 17, 2020

silviuaavram commented Jun 19, 2020

kimroen commented Jun 29, 2020

fix(hooks): add type to on change handler params #985

fix(hooks): add type to on change handler params #985

Conversation

silviuaavram commented Apr 4, 2020 • edited Loading

codecov-io commented Apr 4, 2020

Codecov Report

codecov-commenter commented May 31, 2020

Codecov Report

kimroen commented Jun 6, 2020

silviuaavram commented Jun 7, 2020

kimroen commented Jun 7, 2020

MikeAliG commented Jun 11, 2020

silviuaavram commented Jun 13, 2020 • edited Loading

kimroen left a comment

Choose a reason for hiding this comment

kimroen Jun 16, 2020

Choose a reason for hiding this comment

silviuaavram Jun 16, 2020

Choose a reason for hiding this comment

kimroen Jun 16, 2020

Choose a reason for hiding this comment

silviuaavram Jun 17, 2020

Choose a reason for hiding this comment

silviuaavram commented Jun 19, 2020

kimroen commented Jun 29, 2020

silviuaavram commented Apr 4, 2020 •

edited

Loading

silviuaavram commented Jun 13, 2020 •

edited

Loading