rooks
rooks copied to clipboard
feat!(use-map-state): Actually uses a `Map`
Warning - breaking changes
Added type parameters to the function.
Used useState
with the controls object instead of individual useCallback
s.
Removed has
control because it is readonly an Map.prototype.has()
can be used.
Updated tests to use Map
.
Removed unnecessary checks in the tests.
Updated docs to use new args and return type of hook.
Added pnpm-lock.yaml
because I used pnpm i
.
@ChocolateLoverRaj Pretty solid but I have a few concerns.
- I don't want to introduce breaking changes at this point.
- I don't want to introduce pnpm changes or unrelated typescript changes in this PR as that is a different topic.
But your idea is really good as this allows us to use Map. I personally want useMapState
to be usable with object
s too as they are pretty map-like.
So how do you feel about making this hook work for both object
s and map
s? We can do a call and work on this together if you want to.
I want to really make sure your changes go into main
as I like your passion and this idea is also pretty good.
@imbhargav5 Based on your feedback I will do the following things:
- Remove
pnpm-lock.yaml
and add it to.gitignore
- Put the typescript change in a seperate commit? or PR? The
downleveliteration
is not unrelated because it is needed to rest spread a map like[...map]
. - Keep backwards compatibility by using the old hook if an object is given initially, and use the new hook if a map is given initially. It might get confusing though because then the hook will have two different overloads. It would make sense if the current hook was renamed to
useObjectState
or something other thanuseMapState
. I think we should do both, and have auseObjectState
and auseMapState
. For backwards compatibility, theuseMapState
will work with objects, but for clarity, people can start usinguseObjectState
when writing new code. Then maybe if there are breaking changes in the future,useMapState
can deprecate the feature of working with maps.
Hmm. Yeah. The overloads will be confusing but I think useMapState
should be able to work with Maps
and Objects
since they are pretty similar in Javascript. That is just a preference but we can take a couple of days to think about it. I want to make sure we make the right call.
@imbhargav5 One improvement I made to the Map
version is that instead of returning [map, controls]
, it just returns map
, and the map methods which modify the map use setState
instead. This makes it more convenient.
@imbhargav5 Can you review https://github.com/imbhargav5/rooks/pull/531/commits/9fd1c8223de0f7e1064a14d26f8a2c150f7dedec?
For now I'm using rooks by using github:ChocolateLoverRaj/rooks#build/preview-531
as my rooks
version.
@qiweiii please take a look at this one.
@ChocolateLoverRaj, https://github.com/imbhargav5/rooks/pull/531/commits/9fd1c8223de0f7e1064a14d26f8a2c150f7dedec looks great to me, the only thing is that it only accepts Map type as initalValue
.
I was thinking how to make it work with both Object and Map, and also make it more backwards compatible, so a solution I want to suggest is this:
- Check if the
initialValue
is an object literal or an object created withnew Object()
, usingObject.getPrototypeOf(obj) === Object.prototype
- If it is, we can use
new Map(Object.enties(initialValue))
to turn it into a Map first
reference:
- https://stackoverflow.com/a/43773857/9055088
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries#converting_an_object_to_a_map
@ChocolateLoverRaj just need to resolve the conflict first, and I think it's good to merge, @imbhargav5
Got it @qiweiii . I will review this today or tomorrow.
Codecov Report
Merging #531 (f345019) into main (a50dd21) will increase coverage by
0.20%
. The diff coverage is80.95%
.
@@ Coverage Diff @@
## main #531 +/- ##
==========================================
+ Coverage 72.93% 73.14% +0.20%
==========================================
Files 66 69 +3
Lines 1319 1359 +40
Branches 265 267 +2
==========================================
+ Hits 962 994 +32
- Misses 356 364 +8
Partials 1 1
Impacted Files | Coverage Δ | |
---|---|---|
src/hooks/useMapState/useMapState.ts | 75.00% <75.00%> (ø) |
|
src/hooks/useMapState/useMapOrObjectState.ts | 90.90% <90.90%> (ø) |
|
src/hooks/useMapState/index.ts | 100.00% <100.00%> (ø) |
|
src/hooks/useMapState/useObjectState.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 a50dd21...f345019. Read the comment docs.
New work for this PR in #1284