josepot commented 3 years ago

I think that createKeyedSignal is by far one of the best additions that we've made to the utils package. However, the "mapper" overload is not very ergonomic, for 2 reasons:

When writing the key selector, TS hasn't been able to infer the type returned by the mapper yet
In many cases what's desirable is to create an Observable of a mapped value that doesn't have the key

That's why I think that overload should be improved. Unfortunately, that implies a breaking change :confused:, luckily, though, we are still under major version zero :see_no_evil:

codecov[bot] commented 3 years ago

Codecov Report

Merging #181 (7194175) into main (ca8bfae) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #181   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           28        28           
  Lines          430       432    +2     
  Branches        53        53           
=========================================
+ Hits           430       432    +2

Impacted Files	Coverage Δ
packages/utils/src/contextBinder.ts	`100.00% <ø> (ø)`
packages/utils/src/internal-utils.ts	`100.00% <ø> (ø)`
packages/utils/src/createKeyedSignal.ts	`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 ca8bfae...7194175. Read the comment docs.

josepot commented 3 years ago

Closing this in favor of #182 , since #182 accomplishes the same thing without breaking changes.

re-rxjs / react-rxjs

feat(utils): `createKeyedSignal` improve mapper overload #181

Codecov Report