Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use Maps to only iterate through slices that actually have a given action #2

Merged
merged 4 commits into from
Apr 21, 2024
Merged

use Maps to only iterate through slices that actually have a given action #2

merged 4 commits into from
Apr 21, 2024

Conversation

EskiMojo14
Copy link
Contributor

i saw a FIXME and got an itch

there isn't really a test suite yet as far as i can tell, but i tried it out with the example and it still worked correctly

@EskiMojo14
Copy link
Contributor Author

out of curiosity, is calling set multiple times particularly inefficient? you could always do the iteration inside a single set call instead, building out the object as you go

@dai-shi
Copy link
Member

dai-shi commented Apr 19, 2024

wow, nice! thanks for working on it.
yeah, let's add some tests first. would you like to work on it? using @testing-library/react and testing at the component level would be good.

right, calling set multiple times isn't super efficient (it's still render optimized, though.)

@EskiMojo14
Copy link
Contributor Author

sure - do you want a separate PR for tests or should I just add them to this one?

@EskiMojo14
Copy link
Contributor Author

opened #3 with tests

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Thanks for your work.

for (const [actionName, actionFn] of Object.entries(config.actions)) {
let actionsBySlice = sliceMapsByAction.get(actionName);
if (!actionsBySlice) {
sliceMapsByAction.set(actionName, (actionsBySlice = new Map()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I like this syntax. I think I used two statements before for this pattern.

@dai-shi dai-shi merged commit 077a757 into zustandjs:main Apr 21, 2024
1 check passed
@EskiMojo14 EskiMojo14 deleted the map-of-maps branch April 21, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants